Skip to content

Conversation

georgemccabe
Copy link
Collaborator

@georgemccabe georgemccabe commented Jan 26, 2024

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

  • Describe testing already performed for these changes:

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

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

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.

…or logs if any of the 'Save error logs' steps ran successfully
This reverts commit 7a0a99c.
@georgemccabe georgemccabe added this to the METplus-6.0.0 milestone Jan 26, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 7662047088

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.762%

Totals Coverage Status
Change from base Build 7645730422: 0.0%
Covered Lines: 8508
Relevant Lines: 9374

💛 - Coveralls

Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a 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.

@georgemccabe
Copy link
Collaborator Author

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.

@georgemccabe georgemccabe merged commit f45f1b1 into develop Jan 26, 2024
@georgemccabe georgemccabe deleted the feature_met2796_fix_artifacts branch March 20, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

3 participants