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

Bump go.mod version to v4 #966

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

pbusko
Copy link
Contributor

@pbusko pbusko commented Jul 8, 2024

Summary

To reflect the actual major version and follow the official recommendation regarding go.mod versioning

Use Cases

https://go.dev/doc/modules/version-numbers

A major version update to a number higher than v1 will also have a new module path. That’s because the module path will have the major version number appended, as in the following example:

module example.com/mymodule/v2 v2.0.0

A major version update makes this a new module with a separate history from the module’s previous version. If you’re developing modules to publish for others, see “Publishing breaking API changes” in Module release and versioning workflow.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@pbusko pbusko requested a review from a team as a code owner July 8, 2024 08:09
@pbusko pbusko requested review from mhdawson and brayanhenao July 8, 2024 08:09
@pacostas
Copy link
Contributor

pacostas commented Jul 9, 2024

Hello @pbusko 👋 ! Thank you for your contribution :) Can you describe how consuming the versioned node-engine from the node-engine module will benefit the node-engine module? I'm asking for a little bit of context as this change makes sense for consuming external modules e.g. packit (where in that way you ensure nothing breaks), but I can not find a scenario where the same module would benefit from not consuming the latest version of itself. Also, seems that this change is adding an extra step during development, as for consuming the latest version of the node-engine you will have to make a publish first.

@pbusko
Copy link
Contributor Author

pbusko commented Jul 10, 2024

Hi @pacostas

but I can not find a scenario where the same module would benefit from not consuming the latest version of itself.

it always should, you're right. The proper module versioning is suggested by the official documentation regardless of the usage pattern.

I'm asking for a little bit of context as this change makes sense for consuming external modules e.g. packit

It also makes sense for the installation. At the moment it is impossible to compile the buildpack using the go install command:

$ go install github.com/paketo-buildpacks/[email protected]                                                                                                                                                         1
go: github.com/paketo-buildpacks/[email protected]: github.com/paketo-buildpacks/[email protected]: invalid version: module contains a go.mod file, so module path must match major version ("github.com/paketo-buildpacks/node-engine/v4")

As you can see, the go command requires any go module to follow it's major version.

Some reference to other buildpacks:

https://github.com/paketo-buildpacks/image-labels/blob/bcd2c1314122dd71da5f2cc6c59e7f9f78d6cc37/go.mod#L1
https://github.com/paketo-buildpacks/procfile/blob/7d751939775f9eb43b077ddf5b3f26a2288edc54/go.mod#L1
https://github.com/paketo-buildpacks/watchexec/blob/2c355d90445a9c3ee99be3367902b495a5336180/go.mod#L1

@pacostas pacostas added the semver:patch A change requiring a patch version bump label Jul 10, 2024
pacostas
pacostas previously approved these changes Jul 10, 2024
Copy link
Contributor

@pacostas pacostas left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

Looking acrosss different buildpacks in paketo-buildpacks we found a few that manage the majors this way, but most don't seem to.

We will add it for discussion in the next Paketo WG meeting to try to better understand if this was a choice made at some point and what the concensus is on how the versions should be managed across all of the buildpacks as opposed to switching just for the Node.js related buildpacks.

@c0d1ngm0nk3y c0d1ngm0nk3y self-requested a review July 19, 2024 11:11
@c0d1ngm0nk3y
Copy link
Contributor

Looking acrosss different buildpacks in paketo-buildpacks we found a few that manage the majors this way, but most don't seem to.

But what is the alternative? I think it is clear that this is the way go modules should be versioned. Is there some downside in doing it "right"?

@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the bump-go-mod branch 2 times, most recently from 76f5355 to c02df0e Compare July 23, 2024 06:39
@c0d1ngm0nk3y
Copy link
Contributor

@mhdawson If there is no veto, I would merge it. Imho, there is no real reason to do it in any other way than the official way.

@pacostas
Copy link
Contributor

Hello @c0d1ngm0nk3y, @pbusko ! After a recent discussion in the paketo working group meeting, about the versioning of the modules, I'm trying to find out if this would mean further maintenance over the paketo buildpacks without any significant benefit.

Searching over the paketo buildpacks repositories, the node-engine module is not consumed by any other module, so why do we need to maintain versioning if we are not planning to use it in any of the repositories under the paketo buildpacks?
Definetely versioning is considered as a best practice, but it can also be a bad practice if we are not planning to commit on the semver conventions as stated on the documentation. So another questions is how do we ensure the semver convention/expectations and what type of actions should we create and what kind of structure has the repository has to follow to ensure that? Also consider that the structure of this repo is synced with the one from github-config, so probably this is something that all the buildpacks (node, java, python etc.) have to follow.

@pacostas pacostas dismissed their stale review July 23, 2024 12:38

I need to clarify the actual benefits of this PR and in case we need to follow a versioning we need to consider strategy for that.

@pacostas
Copy link
Contributor

pacostas commented Jul 23, 2024

Looking the buildpacks that do have versioning we can see that after that commit there has been no major version release. For example, the image labels repository does not have a major release for over 3 years. Probably due to extra effort in upgrading it is needed, or due to no github actions for doing this version upgrade are available.

Image labels repo
v1 20/7/2020
v2 18/4/2020
v3 2/2/2021
v4 25/11//2021

Another example that has the versioning is the watchexec repo, where due to lack of proper validation, there has been a version mismatch with what is being referenced on the go.mod file https://github.com/paketo-buildpacks/watchexec/blob/v3.0.0/go.mod with the one that is referenced on the release. Should these two versions match? @pbusko

Similar to the image labels, is the procfile where the last major release (3 years ago) was when the versioning commit added.

v5 25/11/2021
v4 2/2/2021
v3 6/11/2020
v2 18/8/2020
v1 4/3/2020

Practically, we lock the modules on a major version due to lack of maintenance, and as a result we use the latest version of this module. So we have the same behaviour (in terms terms of versioning whether we have versions or not) as we always use the latest and in addition we don't respect the semver convention as seems that there should be a major version in the aforementioned modules over the past 3 years.

@c0d1ngm0nk3y
Copy link
Contributor

@pacostas I am afraid, I didn't get your point.

Let's have a look at procfile. They haven't updated the major for nearly three years, right. But the current version is still 5.9.0, so this is still correct, isn't it?

When I use a tool like dependabot or renovate, it will update my dependencies if there is a new release (e.g. 5.10). But this does currently NOT work for node-engine or node-start. Because from a go versioning perspective github.com/paketo-buildpacks/node-engine is referring to v1 and not v4 which is a bit unfortunate.

I see the current state as broken and can not see how this pr can have a negative impact. I agree, it would be desired to keep the versioning up-to-date, but even if not, that would still be better than the current state imho. At least it would work until the next major bump.

@pbusko
Copy link
Contributor Author

pbusko commented Jul 24, 2024

Searching over the paketo buildpacks repositories, the node-engine module is not consumed by any other module, so why do we need to maintain versioning if we are not planning to use it in any of the repositories under the paketo buildpacks?

@pacostas it is needed not only for consumption of the module as a library, but also for a simple compilation, as shown in #966 (comment). Try running:

go install github.com/paketo-buildpacks/[email protected]

Also it breaks tools like dependabot and renovate, as pointed by @c0d1ngm0nk3y

@pacostas
Copy link
Contributor

@pacostas I am afraid, I didn't get your point.

Let's have a look at procfile. They haven't updated the major for nearly three years, right. But the current version is still 5.9.0, so this is still correct, isn't it?

Yes, it is correct, although seems that the major releases are blocked for the last three years on the procfile, due to the versioning addition, by comparing the fact that before the versioning addition, there was almost 2-3 major releases each year and after the versioning addition there is none for the last three years. I guess this is not on the intentions of this PR, so we need to have proper setup which will enable the bump of major versions on the releases.

procfile major releases timeline:
v5 25/11/2021
v4 2/2/2021
v3 6/11/2020
v2 18/8/2020
v1 4/3/2020

When I use a tool like dependabot or renovate, it will update my dependencies if there is a new release (e.g. 5.10). But this does currently NOT work for node-engine or node-start. Because from a go versioning perspective github.com/paketo-buildpacks/node-engine is referring to v1 and not v4 which is a bit unfortunate.

I see the current state as broken and can not see how this pr can have a negative impact. I agree, it would be desired to keep the versioning up-to-date, but even if not, that would still be better than the current state imho. At least it would work until the next major bump.

I'm not saying that is right and I agree with you at some level, although I think its better not having versioning, which indicates that there might be changes and you should take that into account when you write your code, rather than having versioning which might be false either from a semantic perspective e.g. procfile which seems that it should have a major version and it does not or either from the versioning itself e.g. watchexec where the release points at v3 and the versioning is at v2.

@pacostas
Copy link
Contributor

Searching over the paketo buildpacks repositories, the node-engine module is not consumed by any other module, so why do we need to maintain versioning if we are not planning to use it in any of the repositories under the paketo buildpacks?

@pacostas it is needed not only for consumption of the module as a library, but also for a simple compilation, as shown in #966 (comment). Try running:

go install github.com/paketo-buildpacks/[email protected]

I think you can use go install github.com/paketo-buildpacks/node-engine instead. Do you have a repo where this module is needed to be installed?

Also it breaks tools like dependabot and renovate, as pointed by @c0d1ngm0nk3y

I don't see breaking the dependabot somewhere, If there is a failure where the dependabot breaks, can you ping me on this thread, or create an issue?

@pbusko
Copy link
Contributor Author

pbusko commented Jul 24, 2024

Do you have a repo where this module is needed to be installed?

unfortunately these repositories are not open source

I think you can use go install github.com/paketo-buildpacks/node-engine instead. Do you have a repo where this module is needed to be installed?

the result will not be the latest version of the node-engine buildpack.

$ go install github.com/paketo-buildpacks/node-engine/run@latest
$ strings ~/go/bin/run | grep 'node-engine@'

<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/nvmrc_parser.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/build.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/node_version_parser.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/run/main.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/detect.go

I don't see breaking the dependabot somewhere

Here's the list of versions known to both dependabot and renovate, when reference to the node-engine as a go project:

https://pkg.go.dev/github.com/paketo-buildpacks/node-engine?tab=versions

I'm not saying that is right and I agree with you at some level, although I think its better not having versioning, which indicates that there might be changes and you should take that into account when you write your code

@pacostas What is implied by that specifically? The recommendation behind the go module versioning is as simple as just reflecting the major version in the package name. I don't think that it encourages developers to ignore potential breaking changes and be less careful, since it's a little bit irrelevant to the main topic of this PR and should probably never be the case.

@pacostas
Copy link
Contributor

Do you have a repo where this module is needed to be installed?

unfortunately these repositories are not open source

I think you can use go install github.com/paketo-buildpacks/node-engine instead. Do you have a repo where this module is needed to be installed?

the result will not be the latest version of the node-engine buildpack.

$ go install github.com/paketo-buildpacks/node-engine/run@latest
$ strings ~/go/bin/run | grep 'node-engine@'

<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/nvmrc_parser.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/build.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/node_version_parser.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/run/main.go
<HOME>/go/pkg/mod/github.com/paketo-buildpacks/[email protected]/detect.go

I don't see breaking the dependabot somewhere

Here's the list of versions known to both dependabot and renovate, when reference to the node-engine as a go project:

https://pkg.go.dev/github.com/paketo-buildpacks/node-engine?tab=versions

I see, thank you, I didn't know. So even publishing a release (on github) above 2.0.0 does not mean that go module will follow.

I'm not saying that is right and I agree with you at some level, although I think its better not having versioning, which indicates that there might be changes and you should take that into account when you write your code

@pacostas What is implied by that specifically? The recommendation behind the go module versioning is as simple as just reflecting the major version in the package name. I don't think that it encourages developers to ignore potential breaking changes and be less careful, since it's a little bit irrelevant to the main topic of this PR and should probably never be the case.

Knowing that the go module will not follow above version 2.0.0, practically it does not indicate something other than this module is the version 1.x.x. It is as misleading as on the watchexec scenario where it points to version 2 but is on version 3. Similarly, this module currently points on version 1 but is on version 4.

@pacostas
Copy link
Contributor

I've opened an issue, to discuss an implementation we can follow consistently across all buildpacks. paketo-buildpacks/nodejs#913

Feel free to add any context/ideas/suggestions that I might have missed covering.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM but will need a rebase.

@mhdawson
Copy link
Member

mhdawson commented Aug 8, 2024

Based on discussion in paketo-buildpacks/nodejs#913 we'll land these add hock for now. @pbusko can you rebase and then I'll land.

Co-authored-by: Ralf Pannemans <[email protected]>
@pbusko
Copy link
Contributor Author

pbusko commented Aug 8, 2024

Thank you @mhdawson, the PR has been rebased

@mhdawson mhdawson merged commit a225ff0 into paketo-buildpacks:main Aug 8, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants