-
Notifications
You must be signed in to change notification settings - Fork 491
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
base: main
Are you sure you want to change the base?
✨ Add check ID #4021
Conversation
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
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.
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
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
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>
Went back and reset the structs and ran |
This pull request has been marked stale because it has been open for 10 days with no activity |
This pull request has been marked stale because it has been open for 10 days with no activity |
…into 2577-add-check-id
…into 2577-add-check-id
@ashearin we are planning to merge this at the same time that we flip the switch on maintainer annotations, since both require a schema change to the BigQuery data. So going to do both at the same time. |
This pull request has been marked stale because it has been open for 10 days with no activity |
This pull request has been marked stale because it has been open for 10 days with no activity |
…into 2577-add-check-id
9f1ee2b
to
51505be
Compare
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
163482d
to
31154b5
Compare
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.
LGTM thanks!
/scdiff generate License,Vulnerabilities,Code-Review,Dangerous-Workflow,SAST,Pinned-Dependencies,Branch-Protection,Signed-Releases,Maintained,Token-Permissions,CII-Best-Practices,Packaging,Binary-Artifacts,Dependency-Update-Tool,Security-Policy,Fuzzing |
type CheckResult struct { | ||
Name string |
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.
I would've expected the scdiff
invocation above to fail. Going to poke around a little before merge.
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.
Can you add this patch before merge?
diff --git a/cmd/internal/scdiff/app/compare/compare.go b/cmd/internal/scdiff/app/compare/compare.go
index 51dfe62f..7155a97c 100644
--- a/cmd/internal/scdiff/app/compare/compare.go
+++ b/cmd/internal/scdiff/app/compare/compare.go
@@ -49,6 +49,9 @@ func compareChecks(r1, r2 *scorecard.Result) bool {
if r1.Checks[i].Name != r2.Checks[i].Name {
return false
}
+ if r1.Checks[i].ID != r2.Checks[i].ID {
+ return false
+ }
if r1.Checks[i].Score != r2.Checks[i].Score {
return false
}
This pull request has been marked stale because it has been open for 10 days with no activity |
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 fieldalignment on the files where I made changes to struct members.
What is the current behavior?
Json output only has name for identifying the check
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.
Which issue(s) this PR fixes
Fixes #2577
Special notes for your reviewer
Does this PR introduce a user-facing change?