-
Notifications
You must be signed in to change notification settings - Fork 7
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
set default values for maintainer and vendor to hashicorp #14
Conversation
fpm_template.go
Outdated
@@ -136,7 +139,8 @@ arch: {{ .Arch }} | |||
platform: linux | |||
release: 1 | |||
version: {{ .Version }} | |||
maintainer: {{ .Maintainer }} | |||
maintainer: "HashiCorp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of hardcoding these here - we would want to add something to the above section for inputVendor
and inputMaintainer
to set these to HashiCorp
unless a value has been passed in:
inputVendor := os.Getenv("INPUT_VENDOR")
if inputVendor == "" {
inputVendor = "HashiCorp"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal opinion on this is we should set it as close to user input as possible so it is surfaced easier. So for example, rather than having the empty check in go code here, set the default in the input file https://github.com/hashicorp/actions-packaging-linux/pull/14/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R33. This action should primarily be for us at HashiCorp to use internally but it seems more intuitive that users would check the input file because that's their interface with the action, rather than dig into the template file to check what the implementation details are and find out we are overwriting empty strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the default is defined in GHA instead of the tool itself.
I like that. It also addresses the issue of opting out of the tag if one were to re-use the tool elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -28,6 +28,10 @@ inputs: | |||
description: 'Maintainer name.' | |||
default: '' | |||
required: false | |||
vendor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add this to the inputs
table in the readme too!
README.md
Outdated
@@ -10,6 +10,7 @@ This is a GitHub Action wrapper around nFPM, used to produce DEBs and RPMs. | |||
| `arch` | Build architecture. | | | |||
| `version` | Product semver version. | | | |||
| `maintainer` | Maintainer name. | | | |||
| `vendor` | Vendor name. | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashiCorp
should be in the default column here 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes, thank you for that catch!
0103a7f
to
a4f550d
Compare
a4f550d
to
3a0c55f
Compare
Set
maintainer
andvendor
to HashiCorp to be able to show that HashiCorp is the maintainer/vendor of our linux packages.