-
Notifications
You must be signed in to change notification settings - Fork 2.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
Added the scorecard github action and its badge #16538
Conversation
Signed-off-by: harshitasao <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
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.
Thanks, @harshitasao! I only had a few minor questions and suggestions. I'll approve since we still need a second approver before merging anyhow.
schedule: | ||
- cron: '45 9 * * 3' | ||
push: | ||
branches: [ "main" ] |
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.
We do many pushes (merging a PR) to main per week. Can we then do without the scheduled job?
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.
Coming back to this @harshitasao, any reason why we would not want to remove the schedule
trigger and only keep branch protection rules and main?
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.
Agree with others on this. Let's remove the schedule, and run it on push
.
@harshitasao it makes sense to get this reviewed by one of your mentors as well. |
Signed-off-by: harshitasao <[email protected]>
Sorry @deepthi I missed this message. @joycebrum @diogoteles08 @pnacht Could you please review this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16538 +/- ##
==========================================
+ Coverage 68.74% 68.91% +0.16%
==========================================
Files 1556 1562 +6
Lines 199705 200941 +1236
==========================================
+ Hits 137292 138474 +1182
- Misses 62413 62467 +54 ☔ View full report in Codecov by Sentry. |
@harshitasao would you mind merging |
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.
Once CI is green we can go ahead and merge.
EDIT: Actually we'd want to see a commit that removes the scheduled run, and runs on push
instead.
Signed-off-by: harshitasao <[email protected]>
Thank you @harshitasao! |
This seems to be failing on every push to main: https://github.com/vitessio/vitess/commits/main/ The same error each time:
|
Hey everyone. The issue here is that the Scorecard Action doesn't have tags for major versions, only patch versions. So it needs to be
Sorry, this went unnoticed when I reviewed the code before it got merged. |
Yeah, I saw that: https://github.com/ossf/scorecard-action/tags
No worries! We'll get it updated soon. ❤️ |
Still failed on the first run after addressing the action version: https://github.com/vitessio/vitess/actions/runs/10762449229/job/29842796053
Hopefully that's an ephemeral issue though. |
I just checked with the Fulcio team (a Scorecard dependency to publish the results). They were down for some maintenance over the weekend. Could you try rerunning the workflow? It should work now. |
Yeah, I saw that the 502 error went away at some point last night.
Yep, all looks good! https://github.com/vitessio/vitess/actions/runs/10762449229/job/29878904142 |
Description
PR to add the Scorecard GitHub Action and its badge in the README file.
Related Issue(s)
Checklist
Deployment Notes