Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Update GitHub Action for Modern Syntax #402

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Oct 11, 2023

This PR is based off of #401 to honor the start of the effort by @MPV. I am adding to his start the following:

  • Updating the Dockerfile for a smaller base image (and one that builds!) and one that downloads the latest release (and does not need to go build)
  • Adding a local test for the action to run on a pull request
  • The action.yaml should be in the actions subdirectory
  • Updating the action arguments to support command and args. Command is by default "analyze" and args can be whatever the user needs for the command in question. The split isn't required, but I think it's good to provide a bit of structure. Note that arguments for container actions don't need to be in the action.yml, they can be derived from the environment via INPUT_<NAME>
  • Updating docs in the actions/README.md and adding a table with said arguments.

Please let me know what you would like to discuss / change, happy to do that!

@vsoch vsoch mentioned this pull request Oct 11, 2023
Copy link
Contributor

@MPV MPV left a comment

Choose a reason for hiding this comment

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

Yay, great stuff! 🥰

@MPV
Copy link
Contributor

MPV commented Oct 12, 2023

Just a thought:
Does it even need the Dockerfile?

It means consumers will build this image every time one runs this action.

For example, see how Octopilot does this instead:

A lot less code to maintain and consume.

@vsoch
Copy link
Contributor Author

vsoch commented Oct 12, 2023

It's very quick and ensures we control the base environment. What you pointed out was a composite action that runs directly on the host. I think my preference is to maintain what we currently have, and have another PR that follows up with a larger change if desired. Asking for more than that is a bit of scope creep imho.

@MPV
Copy link
Contributor

MPV commented Oct 12, 2023

No worries, of course up to you to decide what makes sense to include and maintain in this context. 👍

As said before, the PR looks good and I guess there's just an approval needed to merge.

@loosebazooka loosebazooka merged commit 0695621 into GoogleContainerTools:master Oct 16, 2023
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants