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

avoid error being logged when checksums.json is not found #4261

Merged
merged 2 commits into from
Sep 9, 2023
Merged

avoid error being logged when checksums.json is not found #4261

merged 2 commits into from
Sep 9, 2023

Conversation

PetrKralCZ
Copy link
Contributor

@PetrKralCZ PetrKralCZ commented May 19, 2023 •
edited by boegel
Loading

(created using eb --new-pr)

edit: fixes #4301

@PetrKralCZ PetrKralCZ mentioned this pull request May 19, 2023
Closed
@boegel boegel added the bug fix label May 23, 2023
@boegel boegel added this to the next release (4.7.2) milestone May 23, 2023
@boegel boegel mentioned this pull request May 23, 2023
Closed
boegel
boegel requested changes May 31, 2023
View reviewed changes
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@PetrKralCZ As discussed:

  • should enhance test_obtain_file in test/framework/easyblock.py to also test use of warning_only option;
    • to run only that specific test, use: python3 -O -m test.framework.easyblock obtain_file
  • should enhance test_checksum_step to verify that log error doesn't happen anymore if there's no checksums.json file;

@boegel boegel modified the milestones: 4.7.3, release after 4.7.3 Jul 5, 2023
@boegel boegel mentioned this pull request Aug 2, 2023
Closed
@boegel boegel changed the title fix error message when checksums set to [None] fix error message when checksums set to [None] Aug 2, 2023
@Flamefire
Copy link
Contributor

I don't see what it does to address #4254 which is rather addressed with #4150

As far as I can tell this only handles the case where the checksums.json file is missing, i.e. it fixes #4301

@boegel
Copy link
Member

boegel commented Sep 9, 2023

I don't see what it does to address #4254 which is rather addressed with #4150

As far as I can tell this only handles the case where the checksums.json file is missing, i.e. it fixes #4301

You're right of course, thanks for the correction!

@boegel boegel changed the title fix error message when checksums set to [None] avoid error being logged when checksums.json is not found Sep 9, 2023
boegel
boegel approved these changes Sep 9, 2023
View reviewed changes
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

good to go now that a test that catches the issue is in place

@boegel boegel merged commit f1cd377 into easybuilders:develop Sep 9, 2023
42 checks passed
Flamefire
Flamefire reviewed Sep 10, 2023
View reviewed changes
test/framework/easyblock.py
# make sure there's no error logged for not finding checksums.json,
# see also https://github.com/easybuilders/easybuild-framework/issues/4301
regex = re.compile("ERROR .*Couldn't find file checksums.json anywhere", re.M)
regex.search(stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

@boegel did you miss to remove this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, that's not supposed to be there anymore...

Cleaning that up in #4345

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boegel boegel boegel approved these changes

@Flamefire Flamefire Flamefire left review comments

Assignees
No one assigned
Labels
bug fix
Projects
None yet
Milestone
4.8.1
Development

Successfully merging this pull request may close these issues.

checksums.json support clutters log with false crash messages
3 participants
@PetrKralCZ @Flamefire @boegel