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

✨ scdiff: add basic compare functionality #3363

Merged
merged 17 commits into from
Aug 26, 2023

Conversation

spencerschrock
Copy link
Member

What kind of change does this PR introduce?

feature

What is the current behavior?

scdiff output is generated but not used

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

result output is read back into the results format
Note: this support is preliminary still

go run cmd/internal/scdiff/main.go generate --repos repos --output=foo
go run cmd/internal/scdiff/main.go generate --repos repos --output=bar
# modify bar to introduce a diff
go run cmd/internal/scdiff/main.go compare foo bar
  pkg.ScorecardResult{
        Repo: pkg.RepoInfo{
-               Name:      "github.com/ossf/scorecard",
+               Name:      "github.com/ossf/scorecardy",
                CommitSHA: "",
        },
        Date:      s"0001-01-01 00:00:00 +0000 UTC",
        Scorecard: {},
        ... // 4 identical fields
  }

results differ
exit status 1
  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Related to #2462, 4/n

Special notes for your reviewer

From the go-cmp documentation:

It is intended to only be used in tests, as performance is not a goal and it may panic if it cannot compare the values. Its propensity towards panicking means that its unsuitable for production environments where a spurious panic may be fatal.

I think this tool is a valid use-case, but curious to hear other opinions.

There's also the issue of the function: ExperimentalFromJSON2, which I'm taking the naming approach from https://github.com/golang/go/issues/34409

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.)


Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
@spencerschrock spencerschrock temporarily deployed to gitlab August 8, 2023 22:51 — with GitHub Actions Inactive
@spencerschrock spencerschrock temporarily deployed to integration-test August 8, 2023 22:52 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #3363 (3fea4bc) into main (9a844ab) will decrease coverage by 6.22%.
The diff coverage is 72.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3363      +/-   ##
==========================================
- Coverage   72.82%   66.61%   -6.22%     
==========================================
  Files         183      185       +2     
  Lines       12989    13145     +156     
==========================================
- Hits         9459     8756     -703     
- Misses       3009     3902     +893     
+ Partials      521      487      -34     

Signed-off-by: Spencer Schrock <sschrock@google.com>
@spencerschrock
Copy link
Member Author

Here's a real example from #2812, simulating if #2882 was reverted:

  pkg.ScorecardResult{
        Repo:      {Name: "github.com/microsoft/typescript"},
        Date:      s"0001-01-01 00:00:00 +0000 UTC",
        Scorecard: {},
        Checks: []checker.CheckResult{
                ... // 2 identical elements
                {Name: "CI-Tests", Score: 10, Reason: "22 out of 22 merged PRs checked by a CI test -- score normalized"..., Details: {}, ...},
                {Name: "CII-Best-Practices", Reason: "no effort to earn an OpenSSF best practices badge detected", Details: {}},
                {
                        Name:    "Code-Review",
                        Version: 0,
                        Error:   nil,
-                       Score:   7,
+                       Score:   0,
                        Reason: strings.Join({
                                "found 9 unreviewed ",
-                               "changesets out of 30 -- score normalized to 7",
+                               "human changesets (30 total)",
                        }, ""),
                        Details: {},
                        Rules:   nil,
                },
                {Name: "Contributors", Score: 10, Reason: "34 different organizations found -- score normalized to 10", Details: {{Msg: {Text: " contributors work for DefinitelyTyped,HearTao,SwitchaBLE,babel,"...}}}, ...},
                {Name: "Dangerous-Workflow", Score: 10, Reason: "no dangerous workflow patterns detected", Details: {}, ...},
                ... // 11 identical elements
        },
        RawResults: {},
        Findings:   nil,
        Metadata:   nil,
  }

results differ
exit status 1

Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
@spencerschrock spencerschrock temporarily deployed to gitlab August 14, 2023 22:59 — with GitHub Actions Inactive
@spencerschrock spencerschrock temporarily deployed to integration-test August 14, 2023 23:00 — with GitHub Actions Inactive
@spencerschrock spencerschrock marked this pull request as ready for review August 15, 2023 16:36
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.

I assume the endgame for scdiff is to run scorecard and then the diff module, eliminating the need to compare structure/format between two JSON outputs, right? As in, we'd expect consistent result format, modulo runtime/API errors?

cmd/internal/scdiff/app/compare.go Outdated Show resolved Hide resolved
cmd/internal/scdiff/app/compare.go Show resolved Hide resolved
cmd/internal/scdiff/app/compare.go Show resolved Hide resolved
@spencerschrock
Copy link
Member Author

spencerschrock commented Aug 23, 2023

I assume the endgame for scdiff is to run scorecard and then the diff module, eliminating the need to compare structure/format between two JSON outputs, right? As in, we'd expect consistent result format, modulo runtime/API errors?

  1. run scdiff generate on a set to produce the golden.
  2. commit golden to repo.
  3. run scdiff generate on proposed changes
  4. run scdiff compare to evaluate changes.
  5. if intentional, run scdiff update to merge changes to golden, which will be committed

being able to restore from JSON allows for 2 to happen, and saves some API calls.

Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
@spencerschrock spencerschrock temporarily deployed to gitlab August 23, 2023 22:08 — with GitHub Actions Inactive
@spencerschrock spencerschrock temporarily deployed to integration-test August 23, 2023 22:08 — with GitHub Actions Inactive
@spencerschrock
Copy link
Member Author

@spencerschrock spencerschrock temporarily deployed to gitlab August 26, 2023 00:50 — with GitHub Actions Inactive
@spencerschrock spencerschrock temporarily deployed to integration-test August 26, 2023 00:50 — with GitHub Actions Inactive
@spencerschrock spencerschrock merged commit b0a96fe into ossf:main Aug 26, 2023
37 of 38 checks passed
@spencerschrock spencerschrock deleted the golden/from-json branch August 26, 2023 01:22
ashearin pushed a commit to kgangerlm/scorecard-gitlab that referenced this pull request Nov 13, 2023
* Add unmarshall func.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* try to parse the details too.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* Compare skeleton.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* add basic comparison func.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* make normalize exported.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* split compare to separate func.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* Add experimental diff output.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* clarify expected format.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* Handle multiple repo results in files.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* add tests for compare.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* clean up result loading logic.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* add doc comments for advancescanners.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* clarify file error string.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* add high level instructions for the command.

Signed-off-by: Spencer Schrock <sschrock@google.com>

---------

Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants