[chore] Refactor goreleaser config generation for distributions #797
Conversation
…tor-releases into fancy-approach-windows-build
|
FYI I'm working on fixing the failing deb/rpm package tests, but this will be mostly some small details on configuration rather than something that changes the structure of the PR. |
douglascamata
changed the title
|
i'm reviewing this but it takes some time 😅 maybe you could squash the commits into groups of commits that make sense to review separately? |
|
@mowies I'm well aware it takes some time, no worries. Let me know if there's anything I can do to help out with the review, besides the squashing you already mentioned. 👍 I'll try to squash some commits in the next hours and see wether I can group them in a way that will make reviewing easier. It's a bit difficult because it's a "one thing lead to the other" situation all the way down. So the commits ended up organized in the way that I was developing, trying things, figuring out what the path forward, etc. |
|
another option would be to add annotate the code with some PR comments where you feel like an explanation would help make it easier for the reviewer |
Move WithDefaultConfigIncluded
…a/opentelemetry-collector-releases into refactor-go-releaser-config
So now the order should be: - consts - global vars - interfaces - structs and their methods - loose funcs
|
@mowies I think I handled all the feedback you gave so far. I will refrain from anything that would require a forced-push from now on so that reviews become easier as you can use the "commits since your last review" feature. Thank you very much for your time and review. 🙇 By the way, I was about to unexport everything that is unnecessarily exported (that's everything except the |
cool, i'll take another look now
that's good, let's rather have a few follow up PRs :) |
There was a problem hiding this comment.
LGTM! I still think you could move all/most interfaces and structs to the top of the file but that's mostly just my taste.
Additional follow ups that I see:
- moving things into separate files where it makes sense
A full test release in your fork would be good to verify that everything works and that the same amount of release assets is produced in the end
| }).Build() | ||
|
|
||
| // k8s distro | ||
| k8sArchs = []string{"amd64", "arm64", "ppc64le", "s390x"} |
There was a problem hiding this comment.
[nitpick] this could be moved to the other hardcoded vars at the top
This PR will split off of #795.
This moves the code for generating the
goreleaserconfiguration for the distros into a more declarative and easy-to-extend form.Why so many changes? 😱
I am completely aware that it represents a big rewrite of this part of code. I tried a few different approaches with the intent of having a minimum change that would allow me to support Windows, but the amount of changes needed always ended up snowballing quickly. Some examples of the reasons for this "snowballing" of code are (non-exhaustive list):
This might not the best code for this job too, but at least I believe it sets up a good structure that we can build upon and iterate on to improve.
I'm very open to hear your feedback and see how we can move this forward!
I can join the Collector SIG call on 2025-01-15 to talk about this if any maintainer finds it more productive.
Questions ⍰
If anyone, please, could help me out in testing that release process works fine with the update goreleaser configuration, it would be aaaaamazing! I don't feel super confident without testing this, of course.