Generate plugin version at build time based on git describe #147
Conversation
|
With this change the version number reported by The goreleaser machinery will also folllow the same logic when using |
torarnv
force-pushed
the
generate-plugin-version-at-build-time
branch
2 times, most recently
from
4cc9b0b to
afcc79e
Compare
|
Hello Tor Arne 👋 Would you mind explaining the rationale behind this change a bit? It seems to me that the Packer plugin works just fine without the additional complexity of detecting a version, or I'm missing something? |
|
Sure 🙂 We currently manage the version of the plugin in two places:
Are you can see, these two are out of sync ( We also have two ways to produce binaries:
Having to maintain the most recent plugin version in two places (both in source and via tags) is cumbersome and easily gets out of sync. As Packer's plugin resolving logic depend on the versions being accurate, this is not ideal. With the approach in this patch the version is always based on the git tag information, removing the need to manually keep The second benefit is that for development builds (where there are commits on top of the latest tag, or the working tree is dirty), we now automatically produce a build with a version that bumps the patch version one step (and add a For comparison, the workflow that we have currently relies on always pushing a commit after every release that bumps The changes to our |
|
How about we just stop updating the version altogether?1 This looks way simpler and solves both of the issues:
The official, non What do you think? 1: setting it to |
I assume you mean we stop updating That's exactly what this patch does 😄 We stop updating the version, we only push new release git tags. If by leaving
How will it do this? Where will it get the version number from? How will it ensure it's up to date with the latest git tag? How will ensure that it is higher than the latest git tag, which is a requirement for Packer to pick it up with its |
|
Are you suggesting we hard-code the
|
For releases, via tags (see https://github.com/hashicorp/packer/blob/1c7930bec11238af88f5c7256baae0abd11be430/packer/plugin-getter/github/getter.go#L198-L200), just as it works right now. For
You don't need to do any of this as the next invocation of |
I assume you mean that we will not just git tag a commit to mark a release, but also continue to use the goreleaser machinery to both produce valid release filenames (which include the version in the filename), and also to feed the git tag version to the build process, as we do today via linker flags, so that
That will not work. The In addition, it doesn't come with any of the other advantages I listed earlier.
To put it another way, is there a problem with this patch? Why not make things simpler to maintain, and consistent? |
Due to this code in |
Neither
Just want to make sure that we're not solving a non-existent problem here. P.S. your idea with build times is nice and somewhat resembles the Golang's pseudo-versions, I just don't think that with this state of things the proposed approach will be better and less complex that simply setting the |
It will when Packer 1.11 is released: https://www.hashicorp.com/blog/predictable-plugin-loading-in-packer-1-11 Which is why I've made this change 🙄
I respect that, but I've also highlighter several flaws with the current approach, and I fail to see how the With this patch, and Packer 1.11 I can |
Interesting, thanks! Perhaps you could've started with that 😅 |
😅 Sorry, yeah, should have done that. It was mentioned in the link to hashicorp/packer#12749 (comment) I posted in the initial reply, but buried quite deep in other stuff 😄 |
torarnv
force-pushed
the
generate-plugin-version-at-build-time
branch
from
afcc79e to
0464229
Compare
|
I hope this satisfies your particular taste of regex soup 🍲 😄 You are right that it is simpler than depending on a generator 👍🏻 |
|
Picking up on the conversion. Just couple historical cents. We previously had a case when we need to merge in changes from the This change in the current state will make any similar future cases harder to merge in. Is there an option to get the same result with a smaller diff? Or maybe upstream it to the template? |
|
The diff for the latest change is quite minimal I would say. Considering the (low) frequency that we do merge in changes from packer-plugin-scaffolding, the (low) chance that these will conflict with this patch, and the (minimal) amount of work needed to resolve those conflicts if they do happen, I don't see it as a big risk. I can volunteer to resolve any such issues if they do come up, if that helps 😄 We've diverged from the scaffolding already, and the scaffolding is anyways meant as a starting point, so I don't see a principal problem with diverging on this specific feature. Especially consider that the scaffolding is set up for a releasing/version model that we do not follow today (bumping |
I think we could do better, see the patch attached in #147 (comment). No need to change anything other than 2 lines in the |
As I've tried to explain now, on several occasions, that patch does not solve it fully. Can you please read what I've written, and consider the parts not solved by your suggestion? I'm fine making changes based on feedback, but this whole process is getting ridiculous. |
|
@fkorotkov Please read the thread as a whole, including my feedback to @edigaryev's suggestion. Thank you. |
Are you referring to #147 (comment)? I think I've missed that indeed. Quoting it here for reference:
|
|
Good morning ☀️ I see four use-cases here:
The goal for this patch was to ensure we are consistently handling these four cases, by following these principles:
The current state of the repository is as follows:
With the patch proposed in #147 (comment), we get:
With the patch proposed in this PR, we get:
Again, my goal has been to be consistent, both in how we pick up the version (from git, rather than git and a file), and how we distinguish release versions (clean git tags) from development versions (dirty tree). I hope that's a goal we generally share. With that goal in mind I think a patch that changes 15 lines seems entirely reasonable. Claiming "we can do better", when the proposed alternative patch fails on 2 or more of the use-cases seems strange, and I'm left with a feeling that this project doesn't want contributions, or doesn't see the value in contributions that go one step further than fixing the immediate issue. I could have easily hacked around this locally, or in a fork, and gone on with my day. I spent time on and contributed a fix that went beyond that because I thought it would improve the project. I hope that was the right choice. If there still are concerns with this patch, or the goal itself, then please let me know, but please consider the use-cases above and how the proposed solution will handle all of them (if that's part of the goal). |
|
Good morning,
Please take minute to reflect on how could this alternative patch address the use-cases that you've described after the fact. Now on to the constructive things:
First of all, thank you for taking your time to list them here. I only wish if this could be the first message in this PR, and not the 30th one 😅 Just one question:
Why do you need this if we already have a Especially in lieu of the upcoming Packer 1.11 release that changes how the prerelease with suffixes like Not supporting this use-case would allow to simplify things further and make less deviations from the |
Can you elaborate? What do you mean?
I thought that the intent was fairly clear from the original patch, or at least the discussion that followed. And that there was some level of trust that I had good reasons for making the change as is. That's not the feeling I'm left with after the hoops I've gone through in this review. If I could make a wish myself, it would be that reviewers assume good intent from the patch author, and consider the possibility that they have missed something if they feel the approach can easily be reduced to a few lines.
I'm not sure I understand the question. A released version (git tag I personally don't "need" that. I added it because I saw it as a goal to be consistent in how we handle versioning when building this project. Do we not care about that? |
Thanks for explaining your reasoning behind this. I'm not sure if we care about that, because the plugin is not normally intended to be built by users (unless they're developing it), and On top of that this might complicate diagnosis of the problems if some user creates an issue and tells us they run version Now that the patchset is much simpler than it was initially we could probably have that too, and easily roll back in case this proves to be a wrong decision. |
|
Thanks @edigaryev! 🎉 @fkorotkov are there any other concerns, or can we land this? |
|
@torarnv would you mind updating your PR to use the new It seems to be more in line with your changes and will also resolve a merge conflict. |
Sure, will have a look later today. It requires updating our |
This is already done as a part of #148, so a rebase should be enough. |
If the git describe is a plain tag, the resulting plugin version matches the tag. If there are commits on top of the most recent tag, or the working directory is dirty, the resulting plugin version is the latest plugin version with the patch version bumped, and the "dev" suffix added. The same logic has been applied to the goreleaser file, so that the releasing logic can be tested with --snapshot
torarnv
force-pushed
the
generate-plugin-version-at-build-time
branch
from
0464229 to
8bc458f
Compare
Ah, excellent. Done |
The go-gitver package looks at the git describe output and resolves a version based on that.
If the git describe is a plain tag, the resulting plugin version matches the tag.
If there are commits on top of the most recent tag, or the working directory is dirty, the resulting plugin version is the latest plugin version with the patch version bumped, and the "dev" suffix added.
The same logic has been applied to the goreleaser file, so that the releasing logic can be tested with --snapshot