-
Notifications
You must be signed in to change notification settings - Fork 38
Feature dtcenter/MET#2796 develop - Fix error log artifact creation #2475
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
Conversation
…or logs if any of the 'Save error logs' steps ran successfully
…ail, ci-run-diff" This reverts commit ff2d1ca.
… case groups have errors" This reverts commit 8106666.
…i-run-diff to test again
This reverts commit 7a0a99c.
…roperly" This reverts commit cb6d0b4.
…roperly" This reverts commit 82aa0e1.
Pull Request Test Coverage Report for Build 7662047088
💛 - Coveralls |
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.
I approve of these changes.
Thanks for linking to the GHA runs that demonstrate this behavior. I clicked through them and see the behavior you describe. I agree that it's unfortunate that this level of complexity is needed. Glad you were able to get it working.
If it proves difficult to maintain in the future, we could revisit the log handling. One option would be stashing the logs for each use case in the output artifact that's already being written. Would probably be simpler, but wouldn't provide a nice summary of all the errors in one tarfile.
The logs are already stored in the output for each use case group. It was cumbersome to have to download the full output for a use case group to get the logs for a single failure, so I set up logic to copy over logs for use cases that have any errors to create a separate artifact that is smaller in size and easier to download. |
I had to include complicated logic to check if any jobs that save error logs ran successfully to determine if error log artifacts should be merged.
Pull Request Testing
Ran a few test runs to ensure correct behavior occurs:
https://github.com/dtcenter/METplus/actions/runs/7659713325 - doesn't run merge because no cases had errors
https://github.com/dtcenter/METplus/actions/runs/7661855137 - some use cases failed and error_logs was created
https://github.com/dtcenter/METplus/actions/runs/7661220745 - all use case succeeded but had diffs - no error_logs should be created/merged
Review code changes
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Do these changes include sufficient testing updates? [Yes]
Will this PR result in changes to the test suite? [No]
Please complete this pull request review by 1/26/2024.