-
Notifications
You must be signed in to change notification settings - Fork 2.2k
ci: add scheduled run of GHA CI #4760
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
- name: Trigger ${{ matrix.wf_id }} workflow on ${{ matrix.branch}} branch | ||
uses: actions/github-script@v7 | ||
with: | ||
github-token: ${{ secrets.GITHUB_TOKEN }} |
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.
Can we explicitly set permissions:
for the token?
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.
As far as I understand, permissions for this token are set in https://github.com/opencontainers/runc/settings/actions, under "Workflow permissions".
Also, the file itself sets permissions to create actions and read contents.
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.
Would this work if we set there: "Read repository contents and packages permissions" and not set "Allow GitHub Actions to create and approve pull requests"?
This is needed by opencontainers#4760. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
No longer a WIP but a draft waiting for #4763 to be merged first. |
@@ -0,0 +1,29 @@ | |||
name: scheduled | |||
on: | |||
schedule: |
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.
Is it not possible to add the cron schedule to the main CI job description? I've done this in other projects and I believe it used to work.
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.
Ah, I guess the problem is that scheduled runs only work on the main
branch not release branches?
Scheduled workflows will only run on the default branch.
In that case I think I would prefer including the scheduled job for the main branch in ci.yml
and explicitly state this workflow is to run on old release branches.
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.
Yes, this is why it's triggered indirectly. I guess we're mostly ok with main since new PRs pop out regularly.
Most problems we have are with the release branches -- like, you're about to cut a .z release and find out that nothing is working, and you need to backport 27 commits (the number is not entirely unrealistic) just for the CI to work again.
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.
It would be nice to have a comment explaining the need for this, since my first thought was why not have the scheduled
section added to ci.yml
.
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.
Added a comment.
trigger-workflow: | ||
strategy: | ||
matrix: | ||
branch: ["main", "release-1.3"] |
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.
Did you want to include 1.2.z as well, since it's still supported?
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 do, but I'd like to merge this first to see if it works and iron out any issues (it works on my fork but I'm not 100% sure it will work here).
Once 1.3 is working fine, I'll add 1.2 (which will need something similar to #4763 first, too).
.github/workflows/validate.yml
Outdated
- name: succeed (not a PR) | ||
if: github.event_name != 'pull_request' | ||
run: echo "Nothing to check here." |
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 guess this is needed because something inside the commit check thing needs to run to be marked as succeeded? In that case, can you add a comment explaining this?
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.
First, I don't like this kludge myself, and so I spent some time to find a better way to do that (such as, adding commit
job as a needs:
to all-done
conditionally), but nothing works. If there's a way that I missed, let me know.
Second, this code is added by a separate commit, with a decent explanation which can be found via git blame.
Here's a copy-paste from the commit message as it is:
gha/ci: allow validate/all-done to succeed for non-PRs
When we run CI not on a pull request, the commit job is skipped, as a
result, all-done is also skipped.To allow all-done to succeed, modify the commit job to succeed for
non-PRs.
I've added another short comment to the .yml
- name: Trigger ${{ matrix.wf_id }} workflow on ${{ matrix.branch}} branch | ||
uses: actions/github-script@v7 | ||
with: | ||
github-token: ${{ secrets.GITHUB_TOKEN }} |
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.
Would this work if we set there: "Read repository contents and packages permissions" and not set "Allow GitHub Actions to create and approve pull requests"?
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.
This mostly LGTM, I'm just curious about permissions and some more comments to avoid my future self some time :)
When we run CI not on a pull request, the commit job is skipped, as a result, all-done is also skipped. To allow all-done to succeed, modify the commit job to succeed for non-PRs. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... or from another job. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is to ensure that our CI is not rotting away even if there are no new PRs or merges. This is especially useful for release branches which tend to cease working over time due to some external reasons. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
from #4760 (comment):
(for some reason I could not answer in there, so copy-pasting the question and replying below) I was writing that a week ago and at that time I
Unfortunately, I've already closed the tabs with the relevant documentation, but I remember it saying something like "A and B or C need to be set". I can't find where I read it, jumping through these pages: |
This is to ensure that our CI is not rotting away even if there are no new PRs or merges. This is especially useful for release branches which tend to cease working over time due to some external reasons.
Update: I was not sure it's possible, but it appears to work on my fork now, but here it needs #4763 to be merged first,
thus the draft status (until #4763 is merged).Update2: dep PR is merged so this one is ready, too.