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

Fix Successful Repro Pipeline Errors #56

Merged
merged 7 commits into from
Aug 30, 2024
Merged

Conversation

CodeGat
Copy link
Member

@CodeGat CodeGat commented Aug 29, 2024

This PR fixes a couple of errors found when a config successfully reproduces, namely:

  • There was a comment saying repro had failed when it had actually succeeded, due to the wrong bash test being used. See 0b7b943 (for PR CI) and b71a244 (for Scheduled CI). See Failed Comment.
  • The pipeline was allowing merging when the version hadn't been updated properly. This was because it was comparing release ('1.0') against dev (''), seeing that the versions were different, and allowing merging. We now check for '', not just null. See 89d6d6c and 6a36a4c. See Workflow Log Annotation.

And fixes up some variables in comments:

Linking failed PRs ACCESS-NRI/access-esm1.5-configs#86 and ACCESS-NRI/access-esm1.5-configs#87
Closes #57

@CodeGat CodeGat added type:bug priority:blocker type:infra Dealing with CI/CD Pipelines labels Aug 29, 2024
@CodeGat CodeGat self-assigned this Aug 29, 2024
@CodeGat CodeGat force-pushed the 120-repro-success-error branch from 79b3938 to 135c62d Compare August 30, 2024 00:11
@CodeGat CodeGat marked this pull request as draft August 30, 2024 00:21
@CodeGat CodeGat force-pushed the 120-repro-success-error branch from 135c62d to 0bf1601 Compare August 30, 2024 00:31
@CodeGat CodeGat marked this pull request as ready for review August 30, 2024 00:37
@CodeGat CodeGat requested a review from aidanheerdegen August 30, 2024 00:43
@aidanheerdegen
Copy link
Member

  • The pipeline was allowing merging when the version hadn't been updated. This was because it was comparing release ('1.0') against dev (''), seeing that the versions were different, and allowing merging. We now check for '', not just null. See 89d6d6c and 6a36a4c.

Bugger. Now I remember we missed the step of merging the version from release back into dev.

Does the new logic mean we never have to do this? I don't think it's a good idea, so perhaps we should special case when the dev is null or empty but release has a version, and insist that the current version is added to dev?

@CodeGat
Copy link
Member Author

CodeGat commented Aug 30, 2024

I think that the solution is to have modified the version to 1.0 in the dev-* branch rather than the release-* branch. But then we'd still have to have added a boring commit to the trigger the on.push.paths=metadata.yaml anyway.

The real solution is to add another trigger to the version bumping stuff on the release-* branch: on.create, and check that it is a release-* branch.

Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

Give it a rip!

@CodeGat CodeGat merged commit 7934e2d into main Aug 30, 2024
@CodeGat CodeGat deleted the 120-repro-success-error branch August 30, 2024 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:blocker type:bug type:infra Dealing with CI/CD Pipelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configs that reproduce are erroneously stating that they need a !bump major because no config exists
2 participants