-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Trigger Safari & Safari Technology Preview runs after epochs #48087
Trigger Safari & Safari Technology Preview runs after epochs #48087
Conversation
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.
A couple of comments, but assuming this works it should be fine to land.
run: |- | ||
python3 tools/ci/check_for_updated_refs.py >> "$GITHUB_OUTPUT" | ||
env: | ||
GIT_PUSH_OUTPUT: ${{ runner.temp }}/git-push-output.txt |
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'd have a preference for passing this via stdin if we can, rather than as an environment variable.
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.
@jgraham both of these or just this one? I was mostly just blindly looking at https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#good-practices-for-mitigating-script-injection-attacks
safari-stable-results: | ||
name: "All Tests: Safari (stable)" | ||
needs: check-workflow-run | ||
if: | | ||
always() && |
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.
Could we have a comment here? I don't know why always()
is needed. The first condition after that makes sense, and I guess the second is "run if we were manually triggered, or if there's an updated ref", but it would be nice to be sure.
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.
Not just manually triggered, also a push to epochs/three_hourly
or triggers/safari_stable
that hasn't come from a GitHub Actions GitHub token (though obviously there's unlikely to be any of the former).
But added a comment about the always()
GitHub Actions doesn't create new workflow runs for events dispatched from a workflow (or, pedantically, using the provided GITHUB_TOKEN), thus we need to manually trigger the Safari and Safari Technology Preview workflows after the epochs workflow has run. While we're at it, we should allow these workflows to be manually run via workflow_dispatch.
fbc3a9e
to
5548705
Compare
FWIW: putting this in the auto-merge queue so we get this running (and find out if there's more breakage); let's file follow-up issues for the remaining work. |
GitHub Actions doesn't create new workflow runs for events dispatched from a workflow (or, pedantically, using the provided GITHUB_TOKEN), thus we need to manually trigger the Safari and Safari Technology Preview workflows after the epochs workflow has run.
While we're at it, we should allow these workflows to be manually run via workflow_dispatch.
Fixes #48031, fixes #48056.