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

sync-tags: add support for v0.x.y tags #210

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

nikhita
Copy link
Member

@nikhita nikhita commented Dec 4, 2019

For kubernetes/kubernetes#84608

Implements changes to the go modules KEP made in
kubernetes/enhancements#1350.

This commit updates sync-tags to publish v0.x.y tags for corresponding
v1.x.y Kubernetes tag. The old kubernetes-1.x.y tags are still also
published. Both kubernetes-1.x.y and v0.x.y point to the same sha.

For v0.x.y:

  • At this tag, go.mod uses v0.x.y tags for dependencies.
  • When this tag is manually specified in a go.mod file, it will be
    retained as v0.x.y.

For kubernetes-1.x.y:

  • At this tag, go.mod uses v0.x.y tags for dependencies. Note: this
    differs from the previous way, where pseudoversions were used.
  • When this tag is manually specified in a go.mod file, go will resolve
    it to the v0.x.y tag, so pseudoversions would never be needed.

Verified locally. A sample run for client-go (and api + apimachinery) is shown below:

/cc @sttts @liggitt @dims
/assign @sttts

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 4, 2019
cmd/sync-tags/main.go Outdated Show resolved Hide resolved
cmd/sync-tags/main.go Outdated Show resolved Hide resolved
cmd/sync-tags/main.go Outdated Show resolved Hide resolved
@nikhita
Copy link
Member Author

nikhita commented Dec 5, 2019

Addressed all comments except #210 (comment). Also, updated the main PR body and commit messages to reflect the new behaviour and added links to a new test run in the PR body.

If --publish-semver-tags is true, we won't use pseudoversions, so the problem described in #210 (comment) is not relevant anymore.

cmd/sync-tags/main.go Outdated Show resolved Hide resolved
cmd/sync-tags/gomod.go Outdated Show resolved Hide resolved
var changed, goModChanged bool
_, err = os.Stat("go.mod")
if os.IsNotExist(err) {
if !goModExists {
if _, err2 := os.Stat("Godeps/Godeps.json"); err2 == nil {
fmt.Printf("Updating Godeps.json to point to %s tag.\n", bName)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we replace Godeps.json with go.mod everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

or is this really still about godeps?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for the case where there is no go.mod but only Godeps.json file in the repo. We don't need it for k8s repos but might be useful for non-k8s ones.

Or we could remove it here and force them to use the godeps branch in this repo, but that'd break compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

does this script still run on 1.14.x, or did we split the job to use the godeps branch for publishing that branch? dropping godeps manipulation seems ok, but I'd do it in a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

does this script still run on 1.14.x, or did we split the job to use the godeps branch for publishing that branch?

The latter. Will remove this in another PR. 👍

cmd/sync-tags/main.go Outdated Show resolved Hide resolved
cmd/sync-tags/main.go Outdated Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Dec 5, 2019

The tags published to https://github.com/prowtest look good

cmd/sync-tags/main.go Outdated Show resolved Hide resolved
cmd/sync-tags/main.go Outdated Show resolved Hide resolved
@nikhita nikhita force-pushed the semver-tags branch 2 times, most recently from 321e614 to 3f3695e Compare December 5, 2019 17:26
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 5, 2019
cmd/sync-tags/main.go Outdated Show resolved Hide resolved
cmd/sync-tags/main.go Outdated Show resolved Hide resolved
@nikhita
Copy link
Member Author

nikhita commented Dec 9, 2019

Just adding an explicit hold until we've tested this thoroughly
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2019
@nikhita
Copy link
Member Author

nikhita commented Dec 9, 2019

Updated with the latest changes, PTAL. The current workflow (as discussed on slack) is:

  1. figure out all the tags we want to create
  2. if any already exist remotely, we're done
  3. if any already exist locally, delete them (and also delete the corresponding go mod cache .mod/.zip files)
  4. update+tidy+commit the go.mod (with the semver tag if we're semver-tagging, with the pseudoversion otherwise), create all tags, then push all tags with --atomic

Note: for 3) we don't overwrite the existing .mod/.info/.zip files in the go mod cache dir while building tags instead because with the current logic, we avoid creating the .mod, etc files if they already exist.

if _, err := os.Stat(goModFile); err == nil {
fmt.Printf("%s for %s is already packaged up.\n", pseudoVersionOrTag, depPkg)
return nil

Even if we tried to keep track of overwriting it once, it isn't possible to do it across multiple runs (for multiple repos) of sync-tags.

@nikhita nikhita force-pushed the semver-tags branch 2 times, most recently from 41f77fc to 89159bb Compare December 10, 2019 04:55
Implements changes to the go modules KEP made in
kubernetes/enhancements#1350.

This commit updates sync-tags to publish `v0.x.y` tags for corresponding
`v1.x.y` Kubernetes tag. The old `kubernetes-1.x.y` tags are still also
published. Both `kubernetes-1.x.y` and `v0.x.y` point to the same sha.

For `v0.x.y`:

- At this tag, `go.mod` uses `v0.x.y` tags for dependencies.
- When this tag is manually specified in a `go.mod` file, it will be
retained as `v0.x.y`.

For `kubernetes-1.x.y`:

- At this tag, `go.mod` uses `v0.x.y` tags for dependencies. Note: this
differs from the previous way, where pseudoversions were used.
- When this tag is manually specified in a `go.mod` file, go will resolve
it to the appropriate pseudoversion.
@nikhita
Copy link
Member Author

nikhita commented Dec 10, 2019

Rebased on top of master (which brought in #211)

@liggitt
Copy link
Member

liggitt commented Dec 10, 2019

/lgtm
/hold pending run against kubernetes-nightly org

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2019
@liggitt
Copy link
Member

liggitt commented Dec 10, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2019
@sttts
Copy link
Contributor

sttts commented Dec 10, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, nikhita, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants