Skip to content

Conversation

@andreas-el
Copy link
Contributor

@andreas-el andreas-el commented Jan 5, 2026

Issue
Resolves #12518

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

  • 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 main --exec 'just rapid-tests')

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: Add backport label to latest release (format: 'backport release-branch-name')

@andreas-el andreas-el force-pushed the explicit_migrations branch from 542606e to f1250df Compare January 5, 2026 13:12
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2026

Codecov Report

❌ Patch coverage is 77.14286% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.55%. Comparing base (a29328d) to head (b04fe58).

Files with missing lines Patch % Lines
src/ert/cli/main.py 33.33% 4 Missing ⚠️
src/everest/bin/visualization_script.py 50.00% 2 Missing ⚠️
src/ert/gui/main.py 0.00% 1 Missing ⚠️
src/ert/storage/local_storage.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12568      +/-   ##
==========================================
- Coverage   90.55%   90.55%   -0.01%     
==========================================
  Files         435      435              
  Lines       29940    29950      +10     
==========================================
+ Hits        27112    27121       +9     
- Misses       2828     2829       +1     
Flag Coverage Δ
cli-tests 37.36% <51.61%> (+0.01%) ⬆️
gui-tests 68.63% <54.83%> (-0.04%) ⬇️
performance-and-unit-tests 73.92% <74.28%> (+<0.01%) ⬆️
test 38.10% <45.71%> (+<0.01%) ⬆️

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.

@andreas-el andreas-el force-pushed the explicit_migrations branch 2 times, most recently from 64f6475 to 83fe903 Compare January 6, 2026 15:10
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 6, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing andreas-el:explicit_migrations (b04fe58) with main (a29328d)

Summary

✅ 22 untouched benchmarks

Add test using explicit storage migration
Add option to postpone storage reload until migration done
Fixup storage migration tests
Avoid migration overwriting version information for backups
@andreas-el andreas-el force-pushed the explicit_migrations branch from 83fe903 to b04fe58 Compare January 7, 2026 09:14
@andreas-el andreas-el added the release-notes:skip If there should be no mention of this in release notes label Jan 7, 2026
@andreas-el andreas-el changed the title Explicit migrations Have storage migrations done explicitly Jan 7, 2026
@staticmethod
def perform_migration(path: Path) -> None:
if LocalStorage.check_migration_needed(path):
with LocalStorage(path, Mode("w"), True) as storage:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot better than what is currently here, but would it be hard to get more separation and make the migration happen without opening a write storage storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps. Regardless I think that should be part of a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Can you set up a followup issue when merging this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@yngve-sk yngve-sk left a comment

Choose a reason for hiding this comment

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

LGTM

@andreas-el andreas-el merged commit 1cbf498 into equinor:main Jan 8, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes:skip If there should be no mention of this in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Storage migrations are too implicit and should be made explicit

3 participants