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

🌱 Refactor checks to use interface and update tests #3055

Closed
wants to merge 4 commits into from

Conversation

naveensrinivasan
Copy link
Member

@naveensrinivasan naveensrinivasan commented May 23, 2023

  • Refactor BinaryArtifacts check to use an interface
  • Update tests to reflect the new BinaryArtifactsCheck interface
  • Replace checks.BinaryArtifacts calls with b.RunCheck calls
  • Add a RunCheck function to the Check interface

Eventually, move all of them to the same interface. This is the first PR.

What kind of change does this PR introduce?

(Is it a bug fix, feature, docs update, something else?)

What is the current behavior?

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

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

Which issue(s) this PR fixes

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


- Refactor BinaryArtifacts check to use an interface
- Update tests to reflect the new BinaryArtifactsCheck interface
- Replace `checks.BinaryArtifacts` calls with `b.RunCheck` calls
- Add a `RunCheck` function to the `Check` interface

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
@naveensrinivasan naveensrinivasan self-assigned this May 23, 2023
@naveensrinivasan naveensrinivasan temporarily deployed to integration-test May 23, 2023 20:37 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #3055 (150b6ae) into main (e0a6d15) will increase coverage by 0.01%.
The diff coverage is 47.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3055      +/-   ##
==========================================
+ Coverage   63.42%   63.44%   +0.01%     
==========================================
  Files         166      166              
  Lines       12461    12647     +186     
==========================================
+ Hits         7904     8024     +120     
- Misses       4109     4176      +67     
+ Partials      448      447       -1     

Copy link
Member

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

Can you speak to the motivation about moving checks to an interface?

checks/binary_artifact.go Show resolved Hide resolved
checks/checks.go Outdated Show resolved Hide resolved
@naveensrinivasan
Copy link
Member Author

naveensrinivasan commented May 23, 2023

Can you speak to the motivation about moving checks to an interface?

The goal is a plugin interface for checks to allow users to run their own checks.

Provide a framework for running custom checks are to be run within the Scorecard.

By enabling this, we could let users write their custom checks and integrate them with the scorecard.

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
@naveensrinivasan naveensrinivasan temporarily deployed to integration-test May 23, 2023 22:35 — with GitHub Actions Inactive
@naveensrinivasan naveensrinivasan force-pushed the naveen/feat/interface/checks branch from 7c3b8fd to a1ac69f Compare May 24, 2023 17:20
@naveensrinivasan naveensrinivasan temporarily deployed to integration-test May 24, 2023 17:21 — with GitHub Actions Inactive
Copy link
Member

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

I think the contents of the PR are in a good state, but I think this is something big enough that there should be some thought put in before any code. At the very least around the interface.

checks/binary_artifact.go Show resolved Hide resolved
attestor/command/check.go Outdated Show resolved Hide resolved
checks/checks.go Outdated Show resolved Hide resolved
@naveensrinivasan
Copy link
Member Author

I think the contents of the PR are in a good state, but I think this is something big enough that there should be some thought put in before any code. At the very least around the interface.

Do you think we need additional reviewers? Can you be specific?

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
@naveensrinivasan naveensrinivasan force-pushed the naveen/feat/interface/checks branch from a1ac69f to 150b6ae Compare May 27, 2023 15:20
@naveensrinivasan naveensrinivasan temporarily deployed to integration-test May 27, 2023 15:21 — with GitHub Actions Inactive
@naveensrinivasan
Copy link
Member Author

I think the contents of the PR are in a good state, but I think this is something big enough that there should be some thought put in before any code. At the very least around the interface.

Do you think we need additional reviewers? Can you be specific?

Moved it to an internal package till it is ready for primetime.

@spencerschrock
Copy link
Member

I think the contents of the PR are in a good state, but I think this is something big enough that there should be some thought put in before any code. At the very least around the interface.

Do you think we need additional reviewers? Can you be specific?

Moved it to an internal package till it is ready for primetime.

Should we close this given the approach in #3095?

@naveensrinivasan
Copy link
Member Author

I think the contents of the PR are in a good state, but I think this is something big enough that there should be some thought put in before any code. At the very least around the interface.

Do you think we need additional reviewers? Can you be specific?

Moved it to an internal package till it is ready for primetime.

Should we close this given the approach in #3095?

Agreed. Thanks

@spencerschrock spencerschrock deleted the naveen/feat/interface/checks branch January 18, 2024 21:31
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