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

⚠️ Add initial Maintainers Annotation parsing #3905

Merged
merged 72 commits into from Apr 23, 2024

Conversation

gabibguti
Copy link
Contributor

@gabibguti gabibguti commented Feb 28, 2024

What kind of change does this PR introduce?

Enables maintainers to write annotations for Scorecard checks in a scorecard.yml file.

For example, to provide a reasoning that binaries are present in the code but are only used for tests. Scorecard will read the scorecard.yml file from the repository's root (in the expected format) and parse the annotated checks to display them in scorecards.dev UI and ignore them in GitHub's security alerts.

Design doc: https://docs.google.com/document/d/1-5NKRciF3qU-vLS4xPk48EDfC8isz0Z9vnvL4OVjwpQ/edit#heading=h.xzptrog8pyxf

What is the current behavior?

Scorecard runs checks over a repository.

What is the new behavior (if this is a feature change)?**

Maintainers can write annotations to these checks providing a reasoning behind a check's low score.

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Related to #1907

Special notes for your reviewer

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

Enables maintainers to write annotations for Scorecard checks and consumers to view these annotations in Scorecard UI.

Copy link
Contributor

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high level:

  1. the annotation package shouldn't need to know about any of the things above it.
  2. I dont want this showing up in the cron yet.

I left a few style comments that can be ignored for now while we discuss approach. I haven't had a chance to play around with the code, just reading it.

README.md Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
pkg/json.go Outdated Show resolved Hide resolved
pkg/json.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
Copy link
Contributor

@raghavkaul raghavkaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually, it makes more sense to me to have []Exemption with Check string; Annotation string, it's a little more verbose but forces the maintainer to be explicit why individual checks are being exempted. This is just a hunch on what will be more usable/readable, so it's not blocking.

checker/check_result.go Show resolved Hide resolved
maintainers_annotation/maintainers_annotation.go Outdated Show resolved Hide resolved
@gabibguti
Copy link
Contributor Author

Conceptually, it makes more sense to me to have []Exemption with Check string; Annotation string, it's a little more verbose but forces the maintainer to be explicit why individual checks are being exempted. This is just a hunch on what will be more usable/readable, so it's not blocking.

It could be. The reason for having multiple checks bind to multiple annotations is to facilitate 2 scenarios: writing that multiple checks are "justified" by the same reason, and to annotate that multiple reasons apply to a single check too. Since both solutions work, I don't mind changing to this approach if you prefer.

@spencerschrock
Copy link
Contributor

I am not sure if the SARIF output is working as expected. It is clearing the runs. Should it clear only the results?

Was runs supposed to be rules? I don't know enough about SARIF to know whether we should still populate runs if there are no relevant results.

pkg/scorecard.go Outdated Show resolved Hide resolved
Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
We log the error to the user but continue execution with empty config.

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
This method is necessary to validate if experimental feature is enabled so it can activate show annotations feature.

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
@gabibguti
Copy link
Contributor Author

Added the last unresolved comments to issue #4048 so we can fix them later after moving on with this first PR of Maintainers Annotation feature.

@spencerschrock
Copy link
Contributor

/scdiff generate Dependency-Update-Tool,SAST,Maintained,Packaging,Signed-Releases,Branch-Protection,Code-Review,Token-Permissions,CII-Best-Practices,License,Pinned-Dependencies,Dangerous-Workflow,Vulnerabilities,Security-Policy,Fuzzing,CI-Tests,Binary-Artifacts

Copy link

pkg/scorecard.go Outdated Show resolved Hide resolved
Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
@spencerschrock spencerschrock changed the title ✨ Maintainers Annotation ⚠️ Add initial Maintainers Annotation parsing Apr 23, 2024
@spencerschrock spencerschrock enabled auto-merge (squash) April 23, 2024 20:07
@spencerschrock spencerschrock merged commit 8789bbb into ossf:main Apr 23, 2024
41 checks passed
@oreillymj
Copy link

For repo's using the github action, can you clarify the config steps.
Do we add
scorecard.yml to the root of our repo
.scorecard.yml to the root of our repo (note the dot prefix in the filename)

or add the exemptions configuration into the existing .github/workflows/scorecard.yml that the github action creates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants