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.

Sign up for GitHub

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

Jump to bottom

Config: do not overwrite when loaded and not migrated #4605

Merged
merged 1 commit into from
Dec 2, 2020
Merged

Config: do not overwrite when loaded and not migrated #4605

merged 1 commit into from
Dec 2, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 1, 2020 •
edited
Loading

Fixes #4606

The Config.from_file classmethod used to load the config.json file
into memory was always writing the contents from memory to disk if it
was read, even if the content was not migrated. This is unnecessary and
so the write is now only performed if the config was migrated.

@sphuber sphuber requested a review from ltalirz December 1, 2020 22:15
@sphuber sphuber mentioned this pull request Dec 1, 2020
Closed
ltalirz
ltalirz previously approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @sphuber !

@codecov
Copy link

codecov bot commented Dec 1, 2020 •
edited
Loading

Codecov Report

Merging #4605 (6fd15b8) into release/1.5.1 (f945dca) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/1.5.1    #4605      +/-   ##
=================================================
- Coverage          79.48%   79.48%   -0.00%     
=================================================
  Files                482      482              
  Lines              35326    35329       +3     
=================================================
+ Hits               28076    28077       +1     
- Misses              7250     7252       +2
Flag Coverage Δ
django 73.65% <100.00%> (-<0.01%) ⬇️
sqlalchemy 72.82% <100.00%> (-<0.01%) ⬇️

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

Impacted Files Coverage Δ
aiida/manage/configuration/config.py 92.23% <100.00%> (+0.14%) ⬆️
aiida/engine/daemon/runner.py 79.32% <0.00%> (-3.44%) ⬇️
aiida/transports/plugins/local.py 81.29% <0.00%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f945dca...6fd15b8. Read the comment docs.

@sphuber
Copy link
Contributor Author

sphuber commented Dec 2, 2020

@ltalirz Since we are going to release a v1.5.1, I am merging this into the release/1.5.1 branch instead. Once released, this will also be merged into develop, which should be somewhere next week

@sphuber sphuber changed the base branch from develop to release/1.5.1 December 2, 2020 06:46
@sphuber sphuber dismissed ltalirz’s stale review December 2, 2020 06:46

The base branch was changed.

@sphuber
Config: do not overwrite when loaded and not migrated
6fd15b8 
The `Config.from_file` classmethod used to load the `config.json` file
into memory was always writing the contents from memory to disk if it
was read, even if the content was not migrated. This is unnecessary and
so the write is now only performed if the config was migrated.
@sphuber sphuber force-pushed the fix/4606/config-file-overwritten branch from fedd9df to 6fd15b8 Compare December 2, 2020 06:48
@sphuber sphuber requested a review from ltalirz December 2, 2020 06:52
@ltalirz
Copy link
Member

ltalirz commented Dec 2, 2020

@sphuber By the way, I wonder whether we should add a test to prevent a regression of this?

Not sure it's necessary

@sphuber
Copy link
Contributor Author

sphuber commented Dec 2, 2020

Not sure it's necessary

I think it is fine not to include one now

@sphuber sphuber merged commit 6d8ec0b into aiidateam:release/1.5.1 Dec 2, 2020
@sphuber sphuber mentioned this pull request Dec 2, 2020
Closed
@sphuber sphuber deleted the fix/4606/config-file-overwritten branch December 2, 2020 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ltalirz ltalirz ltalirz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Config file gets written each time it is read in Config.from_file even when not migrated
2 participants
@sphuber @ltalirz