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

feat: add goreleaser for gt cli #44

Merged
merged 1 commit into from
Oct 20, 2021
Merged

Conversation

dirien
Copy link

@dirien dirien commented Oct 19, 2021

Signed-off-by: Engin Diri engin.diri@mail.schwarz

fixes #45

As discussed in #43 the PR for gt itself.

@dirien dirien force-pushed the feature/goreleaser branch 4 times, most recently from fc803af to b85e202 Compare October 19, 2021 10:12
@dirien dirien requested a review from brumhard October 19, 2021 14:59
@brumhard
Copy link
Contributor

Hi @dirien ty for the PR. I'll look into it on friday.
One thing that I could see already is that the version is not set.
I was using a version.txt file for that so maybe we can just write the tag to that file during the pipeline?
The was is described here: https://blog.carlmjohnson.net/post/2021/how-to-use-go-embed/ and I wanted to replace the -X flags with it.

@brumhard
Copy link
Contributor

Or we do a release commit for every version to set the correct version in the file to make sure go install also has the right version.

Copy link
Contributor

@brumhard brumhard left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Would be great if you could just add a small note to the readme installation instructions that binary installation is also possible.
Would also be great if we could add other targets like brew in a next step but I really like this as a first step 🙂

@brumhard
Copy link
Contributor

Oh and would be great if we could finish the discussion around embedding vs setting the version through -X before merging.

@dirien
Copy link
Author

dirien commented Oct 19, 2021

@brumhard: Added the install notes for the intstalltion of the binaries.

The question with the version: I did not understand 😅. In goreleaser you can pass the version being released to the LDFLAGS -x.

As you saw from the PR, i did not touched this area, and the version will still be collected from the txt file.

@brumhard
Copy link
Contributor

Ty 🙏
Well my only point is that now 2 values in the version output (commit and date) are set during build but the version on the other hand is set in a file.
The reason for using the file was that you could use go install without the special build flags to get the same Version output as with any prebuilt binary.
With the additions of commit and date this is not possible anymore (since they need to be set through build flags). Now the question is do we really need the newly.added information or could we just go with the version tag which also clearly communicates the tag and therefore the commit and date implicitly in the git history. I think I would prefer that to keep go install working.

@dirien
Copy link
Author

dirien commented Oct 19, 2021

@brumhard, ofc i can drop them from the PR. Just let me know, and I remove the two -X from the goreleaser and delete the variables from the versions.go

@brumhard
Copy link
Contributor

@dirien alright then lets keep them out of the PR for now and we can still add them later if we feel the need for it 👍

Signed-off-by: Engin Diri <engin.diri@mail.schwarz>
@dirien
Copy link
Author

dirien commented Oct 19, 2021

@brumhard done and gone :)

@dirien dirien requested a review from brumhard October 19, 2021 22:46
Copy link
Contributor

@brumhard brumhard 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 very much for the great contribution 🙂 now we can show off our mad goreleaser skills in the meeting with Carlos 😄

@dirien
Copy link
Author

dirien commented Oct 20, 2021

@brumhard, Thanks!

if you plan to create a release let me know so I can be around. THE GITHUB_TOKEN should automatically set. But you never know 😄

@dirien dirien merged commit aa029f1 into SchwarzIT:main Oct 20, 2021
@dirien dirien deleted the feature/goreleaser branch October 20, 2021 06:32
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.

feat: add goreleaser for gt cli
2 participants