-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add goreleaser #115
Add goreleaser #115
Conversation
Keeps failing, not sure I can reproduce locally. Debugging... |
@tompizmor any clues on how can I void this stuttering?
I'd rather have something like this:
Maybe I should fork this on my own repo to see how releases would look like in github before we merge in main. To avoid suprises. |
BTW I branched this on my own private repo and I am playing with generating releases there, see how they look, if they autocreate tags, etc |
Now I am blocked on how to make the version setting automation work well with tags. I was thinking we could remove the |
@josvazg, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
3cf8735
to
d61d3cb
Compare
@josvazg, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
4e8e64b
to
699f3c0
Compare
Can't you use ldflags to set the version of the variable as it is done in charts-syncer? |
did you solve this already? actually I don't know where those paths are defined so I would need to check the documentation |
@josvazg, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
Yes, relok8s was using that already for setting the version inside the binary. The problem was that we were using a version file to track the version and fill ldflags, while goreleaser uses the tag. I tried to get rid of the version file, but it was problematic as I could not get a reliable last tag from git.
I think this should work for us to have manual decided releases. |
BTW I noticed that there is something wrong with the ldflags trick here, I am passing the And this is a PITA to test, as I need to do it on a separate private repo, to avoid polluting our releases here. |
Looking at the final release, those paths do not really matter. Only the files are listed, like in the images I pasted. So I think I will not try to fix any of that. |
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
c162528
to
71aef5f
Compare
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
.goreleaser.yml
Outdated
@@ -0,0 +1,35 @@ | |||
# Make sure to check the documentation at https://goreleaser.com | |||
before: | |||
hooks: |
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.
This does not look right. Are we also building the binary in the hook? What for?
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 can try to remove. It will probably work anyways.
Seems goreleaser builds the binaries itself, does not like to rely on makefiles or anything.
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.
done
.goreleaser.yml
Outdated
- -X github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/cmd.Version={{ .Version }} | ||
goos: | ||
- linux | ||
- windows |
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.
windows? We don't support windows do we? https://github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/blob/main/Makefile#L79
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 can remove
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.
Done
.gitignore
Outdated
@@ -8,3 +8,5 @@ build/* | |||
.idea | |||
*.iml | |||
|
|||
|
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.
add a comment indicating that this is a directory of releases
# Releases
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.
done
bump-version.sh
Outdated
@@ -0,0 +1,15 @@ | |||
#!/bin/bash |
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.
Yeah, I guess we need to trust the operator that it's tagging a proper, valid version. We could also create the release a draft so it can be manually checked before releasing.
In any case, I kind of like how Kubeapps suggests to use some tooling to come up with a valid version, it's better than nothing. https://github.com/kubeapps/kubeapps/blob/master/docs/developer/release-process.md#2---create-a-git-tag
in following patches we should document our release process since now it has a manual component.
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.
Accepted although ptal at removing windows support and some other pieces of feedback. Thanks!
.goreleaser.yml
Outdated
binary: relok8s | ||
archives: | ||
- replacements: | ||
darwin: Darwin |
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.
this replacements to add capitalization does not feel right, in fact I do not like the filename resulting of it which uses underscore + Capitalized at the same time.
To me the only two replacements here should be the ones related to the arch
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.
done
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
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.
From a quick check on the current pipeline and taking into account what in my opinion is the end goal (migrate to public CI), in this patch I think we are missing
- Update build job to just build the docker image. In this case it might not make sense to build binaries since it has been delegated to goreleaser.
- Update build job to stop writing the version file and releasing to github
I think we also need to revisit that last relocate-chart job since it's weird that it's not part of the external tests that we already have.
I'll take a peek at the concourse pipeline. Just to make sure I'm tracking the changes, what we want to do going forward is:
The reason I like keeping the |
… with new images Signed-off-by: Pete Wall <[email protected]>
@migmartri yep that sounds reasonable, I will try to figure out the mapping between the pipeline graph and the yaml to make that happen. @petewall yes, the idea for me is to remove in this PR anything from the CI code that we have moved to goreleaser & actions: build binaries and release, just as Miguel was mentioning. The rest can stay for now. |
BTW I am seeing a new transient issue in tests: --- FAIL: TestGroupChangesByChart (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
panic: runtime error: index out of range [0] with length 0 It happens sometimes, maybe it is a race condition? |
Just noticed that @petewall did the CI changes already. |
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
Signed-off-by: Jose Luis Vazquez Gonzalez <[email protected]>
I added docs and fixed the latest cmment by @migmartri . As @petewall has fixed the CI pipeline, we can now mark this as ready to review. |
Add goreleaser to a release GitHub action and disable versioning and release code from runway CI.
Tested on a separate repo to check out how releases would look like in the GitHub releases page.