-
Notifications
You must be signed in to change notification settings - Fork 40
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
Update RC files instead of overwriting them #652
Merged
jlmaurer
merged 20 commits into
dbekaert:dev
from
garlic-os:issue-638-do-not-overwrite-netrc
Jul 12, 2024
Merged
Update RC files instead of overwriting them #652
jlmaurer
merged 20 commits into
dbekaert:dev
from
garlic-os:issue-638-do-not-overwrite-netrc
Jul 12, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- autopep8 - Added type annotations where the language server wasn't inferring them - Turned the check for api_filename into a guard clause
Behavior of this function changed so that it will no longer overwrite .netrc. .netrc may contain more than one set of credentials, so we have to take care not to destroy them while updating the one relevant to RAiDER/GMAO. The more simple RC files that only ever hold one set of credentials will still just be overwritten, but now RAiDER will know when it's dealing with .netrc/GMAO and parse it and only replace the part that pertains to GMAO's host URL.
- test_credentials.py moved to credentials/test_createFile.py - credentials/test_updateFalse.py: verifies behavior of credentials.check_api while update flag is False - (STAR OF THE SHOW) credentials/test_updateTrue.py: verifies correct behavior while update flag is True, including that .netrc credentials are NOT CLOBBERED
- Reduces code redundancy - Makes it trivial to add new models to the tests, and as such ERA5T and MERRA2 are now tested explicitly along with the others in the update flag tests
…/garlic-os/RAiDER into issue-638-do-not-overwrite-netrc
jlmaurer
approved these changes
Jul 9, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@jhkennedy do you want to take a quick look at this before I merge? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This changes the behavior
RAiDER.models.credentials.check_api
to only update the one set of credentials in a.netrc
file pertinent to RAiDER's operation instead of overwriting the file.Motivation and Context
Fixes issue #638 where other credentials in
.netrc
can be lost.How Has This Been Tested?
Unit tests were added that excerise
check_api
for every supported weather model to ensurecheck_api
is behaving correctly, and crucially, that it no longer modifies unrelated.netrc
credentials.Type of change
Checklist: