Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge Everest into ert #8709

Merged
merged 93 commits into from
Sep 19, 2024
Merged

Conversation

oyvindeide
Copy link
Collaborator

@oyvindeide oyvindeide commented Sep 16, 2024

Issue
Closes equinor/everest#67

One question is how to deal with the version, currently I checked in the version file, and we would track it manually.

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i 10 --exec 'pytest tests/unit_tests -n logical -m "not integration_test"')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@oyvindeide oyvindeide force-pushed the merge_everest_patch branch 6 times, most recently from 1a533fd to eca3fb1 Compare September 16, 2024 13:25
@oyvindeide oyvindeide changed the title Merge everest patch Merge Everest into ert Sep 16, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.45%. Comparing base (6ba0e78) to head (31ecc09).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8709      +/-   ##
==========================================
- Coverage   91.48%   91.45%   -0.03%     
==========================================
  Files         343      343              
  Lines       21257    21257              
==========================================
- Hits        19447    19441       -6     
- Misses       1810     1816       +6     
Flag Coverage Δ
cli-tests 38.94% <ø> (-0.01%) ⬇️
gui-tests 72.41% <ø> (ø)
performance-tests 49.27% <ø> (-0.03%) ⬇️
unit-tests 80.08% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oyvindeide oyvindeide force-pushed the merge_everest_patch branch 2 times, most recently from fd29f58 to 5c12d76 Compare September 17, 2024 07:32
@oyvindeide oyvindeide marked this pull request as ready for review September 17, 2024 07:34
@oyvindeide oyvindeide added the release-notes:new-feature Automatically categorise as new feature in release notes label Sep 17, 2024
src/everest/version.py Outdated Show resolved Hide resolved
@yngve-sk
Copy link
Contributor

Maybe future work / out of scope right now, so for now just an open question: How should we run tests on PRs? I guess now we just run all of it for all PRs.

Some different scenarios

  1. PR with a change only in src/ert and/or tests/ert
  2. PR with a change only in src/everest and/or tests/everests, test
  3. PR with changes to both ERT and Everest (I guess here we just run everything)

tests/everest/conftest.py Outdated Show resolved Hide resolved
@frode-aarstad
Copy link
Contributor

Very good. IMO its mergable as is.

We will just need to monitor closely the next few weeks how everything works and fix issues as they show up

@yngve-sk
Copy link
Contributor

Overall LGTM, imo mergeable after fixing up some smaller things:

  • naming/structure stuff from comments above,
  • working build of the keyword reference docs
  • [test_init_no_project_res is skipped when running locally without everest models installed, or everest models is added as a test dependency].

Whatever remains can be fixed post-merge, not expecting/seeing too many obvious potential problems that can't be discovered/fixed after the merge

@frode-aarstad
Copy link
Contributor

Closes equinor/everest#121

@oyvindeide
Copy link
Collaborator Author

Maybe future work / out of scope right now, so for now just an open question: How should we run tests on PRs? I guess now we just run all of it for all PRs.

Some different scenarios

PR with a change only in src/ert and/or tests/ert
PR with a change only in src/everest and/or tests/everests, test
PR with changes to both ERT and Everest (I guess here we just run everything)

This is a good point, this sets it up to do everything, while it is possible to filter so that some workflows only trigger on changes in a sub-directory, if that workflow is required, it will be marked as pending if it does not start, and will block merging. There are options to bypass this, but they come with their own complications.

Copy link
Contributor

@frode-aarstad frode-aarstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! 🚀

@oyvindeide oyvindeide force-pushed the merge_everest_patch branch 2 times, most recently from 6cf1853 to 364e699 Compare September 19, 2024 06:50
@oyvindeide oyvindeide merged commit 8fea58c into equinor:main Sep 19, 2024
50 checks passed
@oyvindeide oyvindeide deleted the merge_everest_patch branch September 19, 2024 12:29
@roliveira
Copy link
Contributor

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:new-feature Automatically categorise as new feature in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge Everest into ERT