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

Add --upgrade to ./hack/update-deps.sh #2133

Merged
merged 1 commit into from
Apr 3, 2020

Conversation

mattmoor
Copy link
Member

@mattmoor mattmoor commented Mar 2, 2020

For Knative, we have bots that update our inter-repo dependencies each weekday morning (PST), which enables us to very quickly detect and resolve breakages.

I hadn't enabled this for Tekton (thus far) because it used go mod, which was foreign enough to me that I avoided it like the plague. However, a number of factors have pushed me in the direction of having a standard ./hack/update-deps.sh mechanism for bumping certain dependencies, which enables us to support go mod (in Tekton as well as our own in the future).

This trivially enables us to enable the bots for Tekton because go mod becomes a hidden implementation detail.

For an example breaking change this would catch quickly: #2126

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 2, 2020
@tekton-robot tekton-robot requested review from a user and vdemeester March 2, 2020 23:18
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 2, 2020
@tekton-robot
Copy link
Collaborator

Hi @mattmoor. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 2, 2020
@mattmoor
Copy link
Member Author

mattmoor commented Mar 3, 2020

@vdemeester thoughts? I'd like to enable our automation to keep you folks closer to HEAD (and us aware of Tekton breakages).

@mattmoor
Copy link
Member Author

mattmoor commented Mar 3, 2020

Triggers needs something similar, but I wanted to discuss things here first 😉

@vdemeester
Copy link
Member

/ok-to-test

(and us aware of Tekton breakages).

This I like. @mattmoor do you plan to have a bot that tries that ? It will only try release version or master ? (I am less a fan of upgrading daily to master knative/pkg but having something that upgrade to new release automatically is something I would like 👼)

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 3, 2020
@mattmoor
Copy link
Member Author

mattmoor commented Mar 3, 2020

@vdemeester This fast forwards branch dependencies, so master would track HEAD and release-0.X would track that release. There isn't a latest-release branch unfortunately.

@vdemeester
Copy link
Member

@vdemeester This fast forwards branch dependencies, so master would track HEAD and release-0.X would track that release. There isn't a latest-release branch unfortunately.

right… we don't have that release-0… branch mechanisms as knative does on its repositories though.
From my understanding, go get -u the.dep/pkg will get the latest "release" (in the go modules sense), but I don't think knative/pkg does any tagging so… it will track master…

@vdemeester
Copy link
Member

Also with the replace rules, I think go get is more or less a no-op (at least on go 1.13 that seemed to be the case, but I might be mistaken)

@mattmoor
Copy link
Member Author

mattmoor commented Mar 3, 2020

@vdemeester Correct, knative/pkg does branching not tagging.

The replace rules would have to drop for this to be effective, you are right that it is a no-op by itself.

That said, IIUC (please correct, I'm a go mod noob) the replace rules only affect updates so unless there is a reason to float the dependency forward, the dependency should remain frozen where it was.

If a breakage were introduced, we would detect it in the automatic PR within a day, and the breakage could either be fixed or the dependency pinned (with replace) to avoid folks hitting this.

@vdemeester
Copy link
Member

That said, IIUC (please correct, I'm a go mod noob) the replace rules only affect updates so unless there is a reason to float the dependency forward, the dependency should remain frozen where it was.

If a breakage were introduced, we would detect it in the automatic PR within a day, and the breakage could either be fixed or the dependency pinned (with replace) to avoid folks hitting this.

So there is two things:

  1. no replace for knative/* and have the bot upgrade it (daily, weekly, …)
  2. if the bot PR fails, someone fix it (by doing a PR) and we are good
  3. we would need to add a replace (or something) at release time (or maybe not) but at least we should make sure we track a released version of knative/pkg on releases.

@mattmoor does that make sense ?

@mattmoor
Copy link
Member Author

mattmoor commented Mar 3, 2020

Generally, yes. I haven't followed what your release cadence is, but we have been doing every 6 weeks with a knative/pkg cut on the 5th week, where we then pin downstream repos to release-0.Y (the next release). @n3wscott has some automation for this built around toml, but we will need to get on the go mod bandwagon at some point, so maybe this is the kick in the pants we need to extend that to Tekton.

@vdemeester
Copy link
Member

Alright, I can see this being useful. We will need to update a bit our release process (automatically creating a release branch and "freeze" the knative/* dependency to a release-* branch).

Let's at least wait for release 0.11 to get done before merging this 👼

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2020
@mattmoor
Copy link
Member Author

mattmoor commented Mar 3, 2020

@vdemeester looking at go.mod, you don't have a lot of control here. Even your release-0.12 "constraint" isn't really a constraint. This script should actually enable you to float and freeze dependencies in a sensible way:

e.g.

FLOATING_DEPS=(
  # Pin to knative "0.13" release, but allow picking up cherry-picks.
  "knative.dev/[email protected]"
  "knative.dev/caching"  # we don't currently cut releases.
  "knative.dev/[email protected]"
)

@mattmoor
Copy link
Member Author

mattmoor commented Mar 3, 2020

/retest

@vdemeester
Copy link
Member

@mattmoor yeah the script currently doesn't upgrade anything by itself, it's a "manual" run of a user to update a dep.

@vdemeester
Copy link
Member

I really like that 👼

@mattmoor
Copy link
Member Author

mattmoor commented Mar 4, 2020

/retest

@mattmoor
Copy link
Member Author

mattmoor commented Mar 5, 2020

I wanted to ping this now that your release seems to be cut. LMK if I should change anything.

@vdemeester
Copy link
Member

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2020
For Knative, we have bots that update our inter-repo dependencies each weekday morning (PST), which enables us to very quickly detect and resolve breakages.

I hadn't enabled this for Tekton (thus far) because it used `go mod`, which was foreign enough to me that I avoided it like the plague.  However, a number of factors have pushed me in the direction of having a standard `./hack/update-deps.sh` mechanism for bumping certain dependencies, which enables us to support `go mod` (in Tekton as well as our own in the future).

This trivially enables us to enable the bots for Tekton because `go mod` becomes a hidden implementation detail.
@mattmoor
Copy link
Member Author

mattmoor commented Mar 9, 2020

So I just used the same process to enable this for our client repository, and you can see it in action here: knative/client#713. I've actually incorporated some of the improvements from that here (e.g. VERSION=...)

This regularly catches cross-repo breakages for us, which lets us fix them while the context is fresh.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/meow

/cc @sbwsg @bobcatfish for validation

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow

/cc @sbwsg @bobcatfish for validation

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot requested a review from bobcatfish March 24, 2020 17:12
@tekton-robot
Copy link
Collaborator

@vdemeester: GitHub didn't allow me to request PR reviews from the following users: for, validation.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/meow

/cc @sbwsg @bobcatfish for validation

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2020
@ghost
Copy link

ghost commented Apr 3, 2020

/lgtm

@tekton-robot tekton-robot assigned ghost Apr 3, 2020
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2020
@tekton-robot tekton-robot merged commit e9f0454 into tektoncd:master Apr 3, 2020
@afrittoli afrittoli added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Apr 30, 2020
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants