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

[QT-687] Rewrite packaging action #18

Merged
merged 3 commits into from
Apr 1, 2024
Merged

Conversation

ryancragun
Copy link
Contributor

@ryancragun ryancragun commented Mar 11, 2024

This is a proposal for a possible v2 of this action. It makes the following changes

  • Rewrite to run as a composite action instead of Docker action. We do this for two primary reasons:
    • Building the action for every packaging workflow is slow. Depending on the runner this could be anywhere from 15 seconds to a minute. This is work that we need not do on every packaging workflow invocation.
    • It relies on the Docker hub which often throws 429 errors in packaging workflows.
  • Use upstream build artifacts instead of building nFPM from source
    • We still require Github with this method but instead of compiling the binary from source we install the pre-built artifact.
    • We no longer depends on Kyle's branch which was already a bit of a security risk since it runs unpinned code from an external repository that is pointing at the binary we're packing. (The bug fix in his branch was fixed upstream already [0])

Because of the new change in implementation we also consider:

  • Whether or not to install Go for the template generator. This allows callers to set up their own Go toolchain if they want.
  • Add testing for this Go behavior.
  • Add new inputs for nFPM version and installation path.

If we can assume that this workflow was only ever run on Linux or macOS runners then it should be fully backwards compatible. If not, it will not work on windows at this time.

[0] https://github.com/goreleaser/nfpm/blob/main/deb/deb_test.go#L217

This is a proposal for a possible v2 of this action. It makes the
following changes

* Rewrite to run as a composite action instead of Docker action. We do
  this for two primary reasons:
  * Building the action for every packing workflow is slow. Depending
    on the runner this could be anywhere from 15 seconds to a minute.
    This is work that we need not do on every packaging workflow.
  * It relies on the Docker hub which often throws 429 errors in
    packaging workflows.
* Use upstream build artifacts instead of building nPFM from source
  * We still require Github with this method but instead of compiling the
    binary from source we install the pre-built artifact.
  * We no longer depends on Kyle's branch which was already a bit of a
    security risk since he's no longer employed at HashiCorp and we're
    not pinning to a Git SHA. (The bug fix in his branch was fixed
    upstream already [0])

Because of the new change in implementation we also consider:

* Whether or not to install Go for the template generator. This allows
  callers to set up their own Go toolchain if the want.
* Add testing for this Go behavior.
* Add new inputs for nFPM version and installation path.

If we can assume that this workflow was only ever run on Linux or macOS
runners then it should be fully backwards compatible. If not, it will
not work on windows at this time.

[0] https://github.com/goreleaser/nfpm/blob/main/deb/deb_test.go#L217

Signed-off-by: Ryan Cragun <[email protected]>
@ryancragun ryancragun requested review from a team, marianoasselborn and emilymianeil March 11, 2024 21:09
action.yml Outdated
INPUT_LICENSE: ${{ inputs.license }}
INPUT_DEPENDS: ${{ inputs.depends }}
INPUT_BINARY: ${{ inputs.binary }}
INPUT_BIN_PATH: ${{ inputs.bin_path }}

Choose a reason for hiding this comment

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

What is the INPUT_BIN_PATH? I'm comparing it to the list of inputs from the README and this looks like it's new.

Copy link
Contributor Author

@ryancragun ryancragun Mar 11, 2024

Choose a reason for hiding this comment

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

I copied it directly from the previous docker vars and missed that it was no longer necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually wrong, it's used by the template generator to set the package destination. I fixed it in my latest commit.

Signed-off-by: Ryan Cragun <[email protected]>
@shore
Copy link
Contributor

shore commented Mar 12, 2024

If we can assume that this workflow was only ever run on Linux or macOS runners then it should be fully backwards compatible. If not, it will not work on windows at this time.

A quick(ish) GitHub search only shows it being used from these contexts, I think we're good on that front and don't need a major version bump.

Copy link
Contributor

@shore shore left a comment

Choose a reason for hiding this comment

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

Thank you, Ryan!

Signed-off-by: Ryan Cragun <[email protected]>
@ryancragun
Copy link
Contributor Author

@ryancragun
Copy link
Contributor Author

@shore / @jeanneryan Any chance we can get this merged (but not tagged) so I can start testing it in Vault? As long we don't update the tag I wouldn't expect it to have much downstream impact unless people are using @main

@shore shore merged commit f20e9ab into hashicorp:main Apr 1, 2024
1 check passed
@shore
Copy link
Contributor

shore commented Apr 1, 2024

@shore / @jeanneryan Any chance we can get this merged (but not tagged) so I can start testing it in Vault? As long we don't update the tag I wouldn't expect it to have much downstream impact unless people are using @main

done!

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.

3 participants