-
Notifications
You must be signed in to change notification settings - Fork 1.9k
ci: Run cron build only on fluent organization #11329
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: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Amin Vakil <[email protected]>
📝 WalkthroughWalkthroughThree GitHub Actions jobs in the cron-unstable-build workflow now include repository owner gating conditions that restrict execution exclusively to repositories owned by 'fluent', preventing these jobs from running on forks or repositories with different owners. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cron-unstable-build.yaml (1)
68-72: Pre-existing issue: Invalid cron schedule reference.This step references a cron schedule
0 24 * * *that doesn't exist in the workflow's schedule triggers (lines 14-17). Additionally, hour 24 is invalid in cron syntax (valid range is 0-23). This step will never execute.Note: This is a pre-existing issue unrelated to the current PR changes.
Do you want me to help identify the correct cron schedule this should reference, or should this step be removed?
🧹 Nitpick comments (1)
.github/workflows/cron-unstable-build.yaml (1)
25-25: Effective gating, but consider explicit conditions on all jobs for clarity.The condition correctly gates the root job, which will cause all dependent jobs to be skipped on forks. This achieves the PR objective.
However, for better explicitness and clarity, consider adding
if: github.repository_owner == 'fluent'to the other build jobs as well:
unstable-build-images(line 85)unstable-build-packages(line 120)unstable-build-windows-package(line 134)unstable-build-macos-package(line 146)This would make the intent immediately clear without needing to trace job dependencies.
🔎 Example refactor for explicit gating
unstable-build-images: + if: github.repository_owner == 'fluent' needs: unstable-build-get-meta uses: ./.github/workflows/call-build-images.yamlApply similar changes to the other build jobs mentioned above.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cron-unstable-build.yaml
🔇 Additional comments (2)
.github/workflows/cron-unstable-build.yaml (2)
100-100: LGTM!The explicit condition here is good practice, making the gating intent clear even though the job would be skipped anyway due to its dependency on
unstable-build-get-meta.
161-161: LGTM!The explicit gating on the release job is appropriate and ensures that release operations never execute on forks, which is especially important given this job uses sensitive secrets (
RELEASE_TOKEN,RELEASE_REPO).
cosmo0920
left a comment
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.
Sounds reasonable. Thank you!
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| unstable-build-generate-matrix: | ||
| if: github.repository_owner == 'fluent' |
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.
No need for these as it depends on unstable-build-get-meta job that will be blocked - I notice as well you missed the unstable-build-images job anyway so it is not consistent.
Run cron-unstable-build workflow only on fluent organization, it isn't needed to be run on forks.
Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.