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 check ID #4021

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

✨ Add check ID #4021

wants to merge 7 commits into from

Conversation

ashearin
Copy link
Contributor

@ashearin ashearin commented Apr 10, 2024

What kind of change does this PR introduce?

Enhancement to our structured results. Adds a static Check ID for all checks, along with updates to check validation process to make sure the ID is unique. Only impacts Json results.

I also ran betteralign on the files where I made changes to struct members. This resulted in changes to structs I did not touch, but I left the changes since they are beneficial changes. Can revert if thats preferred.

What is the current behavior?

Json output only has name for identifying the check

{
    "details": null,
    "score": 10,
    "reason": "no binaries found in the repo",
    "name": "Binary-Artifacts",
    "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#binary-artifacts",
        "short": "Determines if the project has generated executable (binary) artifacts in the source repository."
    }
},

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

Adding a uint as a unique ID allows users to filter/search for checks without string comparison.

{
    "id": 3,
    "details": null,
    "score": 10,
    "reason": "no binaries found in the repo",
    "name": "Binary-Artifacts",
    "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#binary-artifacts",
        "short": "Determines if the project has generated executable (binary) artifacts in the source repository."
    }
},

Which issue(s) this PR fixes

Fixes #2577

Special notes for your reviewer

Does this PR introduce a user-facing change?

Add Check ID 

Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
@ashearin ashearin requested a review from a team as a code owner April 10, 2024 23:20
@ashearin ashearin requested review from naveensrinivasan and spencerschrock and removed request for a team April 10, 2024 23:20
@ashearin ashearin self-assigned this Apr 10, 2024
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
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.

Declaring the ints in the checks.yaml and validating new duplicates seems reasonable to me. Although I'm curious what would happen if a check were deleted, how would we mark its old number as reserved.

I think this change represents an interesting challenge for the cron, as it's something we could technically go and backfill data for. Can we wait on this until after OSS NA?

Since probes/structured results haven't been released yet, I had thought this could be an easy change to probes in the meantime.

I also ran betteralign on the files where I made changes to struct members. This resulted in changes to structs I did not touch, but I left the changes since they are beneficial changes. Can revert if thats preferred.

Note the linter we use currently is fieldalignment (mentioned in the betteralign readme)

go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment
fieldalignment -fix

pkg/json.go Outdated Show resolved Hide resolved
cron/internal/format/json.go Outdated Show resolved Hide resolved
Comment on lines 54 to 55
Details []string `json:"details"`
ID uint `json:"id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

this requires coordinating a schema update for the bigquery table. which is more involved than I have time for this week with OSS NA prep.

pkg/json.go Outdated Show resolved Hide resolved
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
@ashearin
Copy link
Contributor Author

I think this change represents an interesting challenge for the cron, as it's something we could technically go and backfill data for. Can we wait on this until after OSS NA?

No rush from me, will do a bit more cleanup and we can look at it after next week

Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
@ashearin
Copy link
Contributor Author

Note the linter we use currently is fieldalignment

Went back and reset the structs and ran fieldalignment on them.

Copy link

This pull request has been marked stale because it has been open for 10 days with no activity

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

Successfully merging this pull request may close these issues.

Feature: Add a check ID to each check
2 participants