-
Notifications
You must be signed in to change notification settings - Fork 6
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
ARCH-2122 - Adding reusable workflow for validating catalog-info.yml #271
Conversation
f83f1f4
to
ddad49d
Compare
only-validate-if-file-changed: | ||
description: | | ||
THIS ONLY APPLIES IF THE TRIGGER IS PULL_REQUEST OR PULL. | ||
Flag indicating whether the validation should happen on every run or just if the catalog-info.yml file changed. | ||
This is set to false by default, so validation will happen on every run. | ||
If the trigger is workflow_dispatch or schedule, false is an appropriate choice because validation should always run. | ||
For a trigger like pull_request you may want to set this to true so validation only happens if the file changed. | ||
type: boolean | ||
required: false | ||
default: false |
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 wanted to get other's thoughts on this flag (especially @mike-schenk). Initially Mike indicated he wanted a smart reusable workflow that only ran if it needed to.
In this job though there are a lot of steps that have to run to determine if the validation steps in this job should run. It currently has this optional flag that makes it so the validation steps only run if the catalog-info.yml file changed, but it has to do a number of steps to examine all the applicable conditions. Since it's already doing that work, does it make sense to bail on the last two validation/reporting steps if the file didn't change? Does this flag just introduce complexity? Is it worth doing? Would turning off the failures/job summary be enough in that case?
I made a new standalone template and updated im-build-dotnet-ci with another job so they both use this reusable workflow for validating catalog-info.yml. Inside of the ci job it runs at the same time as the other jobs, so it definitely won't be the longest item. I'm not sure if the selective validation is worth it since we're doing work in the reusable job to figure out if it should run. In the standalone template, the sole point is to validate the catalog-info.yml on a scheduled basis, so it shouldn't ever skip those validation steps.
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.
Nah, I'd do the file change detection outside the reusable workflow then only invoke it conditionally.
I want to propose and promote a more efficient and comprehensive way to implement required status checks on PRs— using a singular PR-triggered workflow that just figures out which other workflows are applicable to call-out to, then fails only if any of them fail. It allows PRs that don't change anything significant to pass their required status check quickly so they can be merged without waiting for a whole battery of irrelevant tests and evaluations to run.
I've implemented something I'm pretty happy with in the funding-calculation repo, which I have just now extended to include your reusable workflow: https://github.com/im-funding/funding-calculation/pull/320
The PR creates a new reusable workflow for validating catalog-info.yml files. It has been incorporated into the workflow templates in two ways: as part of the CI build or as a standalone validation workflow.
To see it in action:
pull_request
only-validate-if-file-changed=true
only-validate-if-file-changed=true
only-validate-if-file-changed=false
push
only-validate-if-file-changed=true
only-validate-if-file-changed=true
only-validate-if-file-changed=false
The pull_request and push runs will be the same except for the step where it tries to create a PR comment. That will obviously never run if it's not a pull_request trigger.