-
Notifications
You must be signed in to change notification settings - Fork 87
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
Tag repo with Go compatible version names #100
Conversation
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
pkg/manifest.go
Outdated
@@ -107,5 +108,8 @@ func ReadInManifest(manifestFile string) (model.InputManifest, error) { | |||
if err := validateManifestDependencies(manifest.Dependencies); err != nil { | |||
return manifest, fmt.Errorf("invalid manifest: %v", err) | |||
} | |||
if _, err = semver.StrictNewVersion(manifest.Version); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will fail for 1.5-dev.sha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we name the release 1.5.0-dev.sha? So that all of our versions in the future will be semver.
it's not accurate then. Consider the 1.4 cases. If we named a PR at head of
1.4 to be 1.4.0 then it's wrong, it's after 1.4.2. But we can't just name
it 1.4.3 since we don't know patch release info.
Simply adding patch release info to the repo won't work either since commit
order is not necessarily release order (confusingly) when we have security
patches in play, which is often. I can point you to a slack thread with a
lot of info if you are interested.
If there is a good solution though that would be great
…On Mon, Dec 16, 2019, 10:00 AM Jason Wang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/manifest.go
<#100 (comment)>:
> @@ -107,5 +108,8 @@ func ReadInManifest(manifestFile string) (model.InputManifest, error) {
if err := validateManifestDependencies(manifest.Dependencies); err != nil {
return manifest, fmt.Errorf("invalid manifest: %v", err)
}
+ if _, err = semver.StrictNewVersion(manifest.Version); err != nil {
can we name the release 1.5.0-dev.sha? So that all of our versions in the
future will be semver.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#100?email_source=notifications&email_token=AAEYGXITFN74FTJLPQJRFT3QY6645A5CNFSM4J3FFYWKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPKXVTY#discussion_r358380810>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXLNCVT4CEAUWIREABLQY6645ANCNFSM4J3FFYWA>
.
|
@howardjohn If you can point me to those conversations, that'd be great! But from my understanding, with our current branching strategy, any new PRs that are merged will belong to current patch version + 1. For example, any new PR merged now will be built with version 1.4.3-dev.sha? |
@howardjohn Thanks! Based on the scenarios discussed in the slack chat, I removed the semver check for the input version and moved it to where we tag go module version. |
fix istio/api#1201
The fix does not do double tagging. If a repo needs to be exposed for external use, we will just release it with go version convention.