Skip to content
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

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

harshitasao
Copy link
Contributor

@harshitasao harshitasao commented Aug 5, 2024

Description

PR to add the Scorecard GitHub Action and its badge in the README file.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

Copy link
Contributor

vitess-bot bot commented Aug 5, 2024

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Aug 5, 2024
@github-actions github-actions bot added this to the v21.0.0 milestone Aug 5, 2024
@mattlord mattlord added Component: General Changes throughout the code base Type: Enhancement Logical improvement (somewhere between a bug and feature) and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Aug 6, 2024
Copy link
Contributor

@mattlord mattlord left a 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.

.github/workflows/scorecards.yml Outdated Show resolved Hide resolved
schedule:
- cron: '45 9 * * 3'
push:
branches: [ "main" ]
Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Member

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.

.github/workflows/scorecards.yml Show resolved Hide resolved
.github/workflows/scorecards.yml Outdated Show resolved Hide resolved
.github/workflows/scorecards.yml Outdated Show resolved Hide resolved
.github/workflows/scorecards.yml Outdated Show resolved Hide resolved
@deepthi
Copy link
Member

deepthi commented Aug 16, 2024

@harshitasao it makes sense to get this reviewed by one of your mentors as well.

@harshitasao harshitasao requested a review from dbussink August 23, 2024 12:34
@harshitasao
Copy link
Contributor Author

@harshitasao it makes sense to get this reviewed by one of your mentors as well.

Sorry @deepthi I missed this message.

@joycebrum @diogoteles08 @pnacht Could you please review this PR.

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.91%. Comparing base (4a89749) to head (4f2f11e).
Report is 59 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@frouioui
Copy link
Member

@harshitasao would you mind merging main on top of your branch? This will allow tests to go green.

deepthi
deepthi previously approved these changes Aug 28, 2024
Copy link
Member

@deepthi deepthi left a 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.

@deepthi deepthi dismissed their stale review August 28, 2024 00:02

Needs one more change

Signed-off-by: harshitasao <[email protected]>
@deepthi deepthi merged commit 1131b7b into vitessio:main Aug 28, 2024
128 checks passed
@deepthi
Copy link
Member

deepthi commented Aug 28, 2024

Thank you @harshitasao!

@mattlord
Copy link
Contributor

mattlord commented Sep 7, 2024

This seems to be failing on every push to main: https://github.com/vitessio/vitess/commits/main/

The same error each time:

Error: Unable to resolve action `ossf/scorecard-action@v2`, unable to find version `v2`

@pnacht
Copy link
Contributor

pnacht commented Sep 7, 2024

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

uses: ossf/[email protected]

Sorry, this went unnoticed when I reviewed the code before it got merged.

@mattlord
Copy link
Contributor

mattlord commented Sep 7, 2024

Hey everyone. The issue here is that the Scorecard Action doesn't have tags for major versions, only patch versions.

Yeah, I saw that: https://github.com/ossf/scorecard-action/tags

Sorry, this went unnoticed when I reviewed the code before it got merged.

No worries! We'll get it updated soon. ❤️

@mattlord
Copy link
Contributor

mattlord commented Sep 8, 2024

Still failed on the first run after addressing the action version: https://github.com/vitessio/vitess/actions/runs/10762449229/job/29842796053

Using payload from: results.json
Generating ephemeral keys...
Retrieving signed certificate...
2024/09/08 19:15:09 error signing scorecard results: getting key from Fulcio: retrieving cert: POST https://fulcio.sigstore.dev/api/v1/signingCert returned 502 Bad Gateway: "<!doctype html><meta charset=\"utf-8\"><meta name=viewport content=\"width=device-width, initial-scale=1\"><title>502</title>502 Bad Gateway"
2024/09/08 19:15:09 retrying in 10s...
2024/09/08 19:15:19 error signing scorecard json results: error signing payload: getting key from Fulcio: retrieving cert: POST https://fulcio.sigstore.dev/api/v1/signingCert returned 502 Bad Gateway: "<!doctype html><meta charset=\"utf-8\"><meta name=viewport content=\"width=device-width, initial-scale=1\"><title>502</title>502 Bad Gateway"

Hopefully that's an ephemeral issue though.

@pnacht
Copy link
Contributor

pnacht commented Sep 9, 2024

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.

@mattlord
Copy link
Contributor

mattlord commented Sep 9, 2024

I just checked with the Fulcio team (a Scorecard dependency to publish the results). They were down for some maintenance over the weekend.

Yeah, I saw that the 502 error went away at some point last night.

Could you try rerunning the workflow? It should work now.

Yep, all looks good! https://github.com/vitessio/vitess/actions/runs/10762449229/job/29878904142

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: General Changes throughout the code base Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable OpenSSF Scorecard to enhance security practices across the project
7 participants