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

image: Build the scorecard command from this repo's wrapper instead of copying #316

Closed
wants to merge 2 commits into from

Conversation

justaugustus
Copy link
Member

PRs like #312 and #313 demonstrate that it's possible to drift between what is built for the scorecard action container image and what dependencies are in the Golang-based action, which means the behavior can be unpredictable and can lead to breakages, as seen in #277.

This PR is a small change, which ensures we only need to update the scorecard dependency in a single place.
I also added some baseline testing docs, as a treat!

Commits:

  • image: Build the scorecard command from source instead of copying

    This is another step in getting rid of the bash-based entrypoint.
    By building from the scorecard-action source, we're ensuring a few
    things:

    • The existing entrypoint leverages the wrapper code
    • We only track the scorecard dependency in a single location (go.mod)
  • image: Include event paths for testing scenarios

Signed-off-by: Stephen Augustus [email protected]

This is another step in getting rid of the bash-based entrypoint.
By building from the scorecard-action source, we're ensuring a few
things:
- The existing entrypoint leverages the wrapper code
- We only track the scorecard dependency in a single location (go.mod)

Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus justaugustus enabled auto-merge (squash) May 25, 2022 01:20
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #316 (eb836e4) into main (5c8bc69) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #316   +/-   ##
=======================================
  Coverage   60.47%   60.47%           
=======================================
  Files           4        4           
  Lines         210      210           
=======================================
  Hits          127      127           
  Misses         74       74           
  Partials        9        9           

@justaugustus
Copy link
Member Author

(I'm going to save any thoughts I had about being clever with the bash script for a future PR to keep the diff small.)

@laurentsimon
Copy link
Contributor

One worry is time to run the Action. Compiling scorecard takes several minutes, so it would be great to use pre-build containers. We can compile the container using the Dockefile you've updated.

How about temporarily renaming this Dockefile to Dockerfile.go? Otherwise the e2e tests will start breaking immediately. We have this issue #236 that needs to be resolved before we can move to the go code

Wdut?

1 similar comment
@laurentsimon
Copy link
Contributor

One worry is time to run the Action. Compiling scorecard takes several minutes, so it would be great to use pre-build containers. We can compile the container using the Dockefile you've updated.

How about temporarily renaming this Dockefile to Dockerfile.go? Otherwise the e2e tests will start breaking immediately. We have this issue #236 that needs to be resolved before we can move to the go code

Wdut?

@azeemshaikh38
Copy link
Contributor

The Dockerfile changes should be pushed to golang-staging branch. That'll automatically test this change for Golang action against the current bash action. Apart from that LGTM.

@spencerschrock
Copy link
Member

This PR is old and appears to have been addressed by #750

auto-merge was automatically disabled July 7, 2023 18:41

Pull request was closed

@justaugustus
Copy link
Member Author

This PR is old and appears to have been addressed by #750

Thanks for catching this and cleaning up, @spencerschrock!

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.

4 participants