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

Chksum fixes maint 5.6 #3038

Merged
merged 5 commits into from
Mar 11, 2019
Merged

Conversation

jedwards4b
Copy link
Contributor

Fix chksum issues - only download chksum file(s) if requested, look on all servers in config_inputdata.xml for chksum files and merge those found.

Test suite: scripts_regression_tests.py
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #3033
Fixes #3034
Fixes #3036

User interface changes?:

Update gh-pages html (Y/N)?:

Code review:

@jgfouca
Copy link
Contributor

jgfouca commented Mar 11, 2019

@jedwards4b , I'd like to have this fix in CIME master before I merge back to E3SM. Will you be doing a maint merge any time soon?

@jedwards4b
Copy link
Contributor Author

Yes we can do it as soon as this is merged.

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

One minor inline comment.

More generally, though: I can see how this addresses point (2) from #3034, but I don't see how this addresses point (1) from that issue. Can you please point me to the changes that address that first point?

@@ -137,7 +137,8 @@ def check_all_input_data(self, protocol=None, address=None, input_data_root=None
inputdata = Inputdata()
protocol = "svn"
success = False
while not success and protocol is not None:
# download and merge all available chksum files.
while protocol is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Since you no longer use the success value returned from the call to _download_checksum_file, you should probably change that to _ to signal that the return value isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I'll wait to approve until you answer my more general question.

@jedwards4b
Copy link
Contributor Author

@billsacks - fixed now.

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @jedwards4b . This mostly looks good to me now.

One small remaining thing: The meaning of the status return value from _download_checksum_file is not very clear, now that it loops over servers. Does True indicate that all downloads were successful, or that at least one was, or that the last one was? Can you please either clarify this in the documentation of the function or remove this return (since it doesn't seem to actually be checked anywhere now).

@jedwards4b
Copy link
Contributor Author

@billsacks you are correct, I did miss a case. Fixed now

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@jedwards4b jedwards4b merged commit 170db73 into ESMCI:maint-5.6 Mar 11, 2019
@jedwards4b jedwards4b deleted the chksum_fixes_maint-5.6 branch March 11, 2019 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants