Support simplified building and go get #98
Conversation
|
This would address #88 |
There was a problem hiding this comment.
I like the simplification in general. I left you some comments inline - I'm happy to address these myself next week, but I wanted to share that feedback before touching the code myself.
As a side note I still think there's value in the identification of dev builds and I'd like to address that by adding some form of sed && git commit probably inside GitHub Actions. That might also explain why I think it's good to keep these variables in a separate file. Cutting releases via goreleaser --snapshot is fine, but not everyone's going to do that I think, also because it builds all platforms at the same time.
main.go
Outdated
| version string = `dev` | ||
| commit string = `` | ||
| date string = `` | ||
| builtBy string = `` |
There was a problem hiding this comment.
As discussed offline I'm ok with these variables being in main package - as we can pass them down to wherever they're needed and it is better than global state in version package.
However (1) I think it'd be cleaner to still keep them in a separate file and (2) it would be better to retain some of the logic from the version package:
- variable names
version/prerelease- which can be un-exported, but I think we should retain the meaning and avoid settingversiontodev. I'd say the meaningful default isversion = "0.0.0"prerelease = "dev" - semver check via
init()- which won't bepanicing ongo get/build/installanymore ifversiondefaults to0.0.0, but still gives us safety net against building a version which isn't semver-compatible
There was a problem hiding this comment.
A separate file is a good call.
Why have separate fields? It gets complicated for snapshots as its already going to send a prerelease string to the version (ie. 1.2.3-SNAPSHOT-hash) or if your goreleaser tag is prerelease, it will mess it up wouldn't it? As the version var and the prerelease var will kinda be redundant. Could just do version string = "0.0.0-dev" couldn't you?
There was a problem hiding this comment.
As for GoReleaser - it may do by default, but it won't with custom ldflags, like we have now (without this patch), except that these flags would be less verbose with variables being in main.
As for why - I don't have any better reason other than keeping it aligned with other HashiCorp Go projects (terraform, packer, consul, vault, nomad).
That said I appreciate you challenging this convention and I have not asked teams why they do it and if they still feel this is right in today's context, as the separate package thing probably isn't in retrospect and this may be just a relic of some inflexible release tooling we may still have in place.
So I'll ask around next week - and if there are no strong opinions/reasons then I'm happy to go ahead with the "GoReleaser style" you're proposing! 😄
|
As discussed/clarified via DMs I have opened goreleaser/goreleaser#1532 |
|
goreleaser/goreleaser#1557 will be part of the next GoReleaser release, which means we can leverage https://goreleaser-git-f-snapshot-versioning.goreleaser.now.sh/customization/templates/ We can put this PR hold temporarily until the next release comes out as that should happen 🔜 |
This makes building and installing simpler by relying on default behavior from goreleaser, we can encourage users to use `goreleaser --snapshot` to create local builds if they need them, simplifying the usage story on Windows and allowing this to be go-gettable.
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
ghost
locked and limited conversation to collaborators
I modified the usage of ldflags to support the goreleaser default variables it fills in and removed the
buildtarget from the Makefile. I think in the docs we should just encourage people to usegoreleaser --snapshotlocally, unfortunately you can't seem to skip the Artifactory step currently.Curl / Make are not available by default on Windows, and we should probably make it more approachable for as many devs as possible to contribute. Considering the length of the Makefile now we could also just remove it but we should definitely call out the vendor usage for a canonical test environment.