Skip to content

Add support for prefix of any character#79

Merged
ssagarverma merged 8 commits into
hashicorp:mainfrom
BESTSELLER:master
Mar 30, 2026
Merged

Add support for prefix of any character#79
ssagarverma merged 8 commits into
hashicorp:mainfrom
BESTSELLER:master

Conversation

@brondum

@brondum brondum commented Oct 9, 2020

Copy link
Copy Markdown
Contributor

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

@hashicorp-cla

hashicorp-cla commented Oct 9, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Gaardsholt

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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.

Comment thread README.md Outdated
@Gaardsholt

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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?

@mohanmanikanta2299

Copy link
Copy Markdown
Contributor

Hey @OrKarstoft ,

We are discussing with our internal teams to identify if it's feasible to extend this feature in this repo and the impact. We will get back to you ASAP.

@OrKarstoft

Copy link
Copy Markdown
Contributor

Hey @OrKarstoft ,

We are discussing with our internal teams to identify if it's feasible to extend this feature in this repo and the impact. We will get back to you ASAP.

Hey @mohanmanikanta2299,

Thanks for letting me know.

@ssagarverma

Copy link
Copy Markdown
Contributor

Hey @OrKarstoft, Thanks for your patience here, and for continuing to engage on this.
We’ve now been able to get alignment internally that an opt-in prefix-based approach is a reasonable direction for supporting this use case without affecting existing behaviour.
I saw your approach and it looks good to me, please go ahead and update this PR (also document these changes) we’re happy to review it in detail and continue from there.
Appreciate the thought and effort you’ve put into this.

@OrKarstoft

OrKarstoft commented Jan 30, 2026

Copy link
Copy Markdown
Contributor

Hey @ssagarverma,

Thank you very much. I've implemented the changes in the PR, along with tests and documentation. Let me know if there's anything you'd like differently.

Comment thread version.go Outdated
Comment thread version_test.go Outdated
Comment thread version_test.go Outdated
Comment thread version_test.go Outdated
Comment thread version.go Outdated

@OrKarstoft OrKarstoft left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ssagarverma Thank you for your review and your patience! I've made some changes as a result of your review, but I have additional questions regarding the cross-prefix versions comparison.

@ssagarverma

ssagarverma commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

@OrKarstoft I’ve added a comment along with a small patch that should help us move this PR toward closure. Really appreciate your continued engagement and patience here 🫡.

Add support for version prefixes in NewVersion and related methods
@OrKarstoft

Copy link
Copy Markdown
Contributor

@ssagarverma Thank you for collaborating on this! I reviewed your PR, and it looks very good.
Do you want to give it a second/third/nintyninth look, or merge it straight away?

Nevertheless, I've enjoyed this contribution, even though it's taken forever.

@ssagarverma

Copy link
Copy Markdown
Contributor

@ssagarverma Thank you for collaborating on this! I reviewed your PR, and it looks very good. Do you want to give it a second/third/nintyninth look, or merge it straight away?

Nevertheless, I've enjoyed this contribution, even though it's taken forever.

Indeed! These things can sometimes take a bit longer due to factors like rotating reviewers and other priorities, so the delay isn’t intentional but more of a side effect of the process. What matters most is arriving at the right solution, even if it takes a few iterations.

The back-and-forth in these review cycles is actually quite valuable, it helps ensure the final change feels natural and easy to adopt for a wide range of users. Cheers ✌️

@ssagarverma ssagarverma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM :)

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.

8 participants