Skip to content

Conversation

@brondum
Copy link

@brondum brondum commented Oct 9, 2020

To support releases with other prefixes than "v", such as the ingress-nginx
https://github.com/kubernetes/ingress-nginx/releases

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 9, 2020

CLA assistant check
All committers have signed the CLA.

@Gaardsholt
Copy link

Is it realistic to get this pull request merged?

@Gaardsholt Gaardsholt requested a review from a team as a code owner December 4, 2025 13:36
@Gaardsholt
Copy link

@hashicorp/team-ip-compliance

I am here once again asking for a review

Signed-off-by: Lasse Gaardsholt <lasse.gaardsholt@bestseller.com>
@CreatorHead
Copy link

It changes what “a version” means

Before:
If your input is wrong → you get an error

After this PR:
If your input has extra text → it might silently succeed

Example:
NewVersion("my-app-v1.2.3")

Before → error (good signal you passed a tag, not a version)
After → success (maybe unintended)

The PR says “any prefix”, but it’s not really “any”

The PR title says “prefix of any character”.
In reality, it only allows:

  • letters
  • numbers
  • underscore
  • followed by -

So these do NOT work:
nginx.ingress-v1.2.3
foo/bar-v1.2.3
release@v1.2.3

So the wording is misleading, and users will assume broader support than actually exists.

go-version has always been strict by design.

This PR:

  • changes what counts as a “valid version”
  • does so globally
  • without opt-in, docs, or migration notes
    That’s not a bug fix, it’s a behavior change, and those must be deliberate and explicit.

My Observation:
This PR changes the rules of what counts as a “version”, but it’s unclear whether that’s intentional, the tests contradict the code, and the change can silently hide bugs, so the behavior needs to be clarified before merging.

go-version has always been strict by design.
This PR:

  • changes what counts as a “valid version”
  • does so globally
  • without opt-in, docs, or migration notes
    That’s not a bug fix, it’s a behavior change, and those must be deliberate and explicit.

README.md Outdated
Comment on lines 6 to 8
> [!IMPORTANT]
> This repository is archived and will not be updated. The repository will be deleted eventually. We suggest you migrate to [`hashicorp/go-version`](https://github.com/hashicorp/go-version)

Choose a reason for hiding this comment

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

Please remove this note.

Choose a reason for hiding this comment

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

Done

@Gaardsholt
Copy link

It changes what “a version” means

Before: If your input is wrong → you get an error

After this PR: If your input has extra text → it might silently succeed

Example: NewVersion("my-app-v1.2.3")

Before → error (good signal you passed a tag, not a version) After → success (maybe unintended)

The PR says “any prefix”, but it’s not really “any”

The PR title says “prefix of any character”. In reality, it only allows:

  • letters
  • numbers
  • underscore
  • followed by -

So these do NOT work: nginx.ingress-v1.2.3 foo/bar-v1.2.3 release@v1.2.3

So the wording is misleading, and users will assume broader support than actually exists.

go-version has always been strict by design.

This PR:

  • changes what counts as a “valid version”
  • does so globally
  • without opt-in, docs, or migration notes
    That’s not a bug fix, it’s a behavior change, and those must be deliberate and explicit.

My Observation: This PR changes the rules of what counts as a “version”, but it’s unclear whether that’s intentional, the tests contradict the code, and the change can silently hide bugs, so the behavior needs to be clarified before merging.

go-version has always been strict by design. This PR:

  • changes what counts as a “valid version”
  • does so globally
  • without opt-in, docs, or migration notes
    That’s not a bug fix, it’s a behavior change, and those must be deliberate and explicit.

I understand the need to be strict in this package.
I'm guessing we don't want a new function for this, like version.NewVersionWithOptions()?

Do you propose that we scrap this PR, or how do we move forward from this?

@OrKarstoft
Copy link

It changes what “a version” means

Before: If your input is wrong → you get an error

After this PR: If your input has extra text → it might silently succeed

Example: NewVersion("my-app-v1.2.3")

Before → error (good signal you passed a tag, not a version) After → success (maybe unintended)

The PR says “any prefix”, but it’s not really “any”

The PR title says “prefix of any character”. In reality, it only allows:

  • letters
  • numbers
  • underscore
  • followed by -

So these do NOT work: nginx.ingress-v1.2.3 foo/bar-v1.2.3 release@v1.2.3

So the wording is misleading, and users will assume broader support than actually exists.

go-version has always been strict by design.

This PR:

  • changes what counts as a “valid version”
  • does so globally
  • without opt-in, docs, or migration notes
    That’s not a bug fix, it’s a behavior change, and those must be deliberate and explicit.

My Observation: This PR changes the rules of what counts as a “version”, but it’s unclear whether that’s intentional, the tests contradict the code, and the change can silently hide bugs, so the behavior needs to be clarified before merging.

go-version has always been strict by design. This PR:

  • changes what counts as a “valid version”
  • does so globally
  • without opt-in, docs, or migration notes
    That’s not a bug fix, it’s a behavior change, and those must be deliberate and explicit.

Hey!

You've made a good point here, so I fiddled and came up with a solution that implements the opt-in functionality without breaking changes. This is what I came up with. Was it something in those lines you were also thinking?

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.

6 participants