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

Bump kubernetes to 1.16.5 and knative/pkg to release-0.12 #1894

Merged
merged 2 commits into from
Feb 3, 2020

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented Jan 17, 2020

Changes

This updates cmd/webhook/main.go to use latest knative/pkg code, most simplified

This also changes the following:

  • Add validating and mutating webhook definition in yaml (so it gets
    registered before the webhook starts)
  • Rename the controller service/deployment from tekton-pipelines-controller to controller
  • Rename the webhook service/deployment from tekton-pipelines-webhook to webhook
  • Use pipeline.tekton.dev/release instead of tekton.dev/release ;
    mainly to let other project use their own (trigger.tekton.dev/release).

/hold waiting for 0.10 to be out to get this merge

Also, this moves the min k8s server version to 1.15.x

Closes #1113

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Tekton Pipelines will now required a k8s cluster 1.15+ (Internal kubernetes dependency is bumped to 1.16)

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jan 17, 2020
@tekton-robot tekton-robot requested review from afrittoli and a user January 17, 2020 16:40
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 17, 2020
@vdemeester
Copy link
Member Author

/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 Jan 17, 2020
@vdemeester vdemeester force-pushed the bump-k8s branch 4 times, most recently from 9e48050 to b7ffda9 Compare January 27, 2020 11:24
@vdemeester vdemeester changed the title Bump kubernetes to 1.16.5 and knative/pkg to release-0.11 Bump kubernetes to 1.16.5 and knative/pkg to release-0.12 Jan 27, 2020
// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
func(ctx context.Context) context.Context {
// FIXME(vdemeester) uncomment that for auto-conversion
// return v1alpha2.WithUpgradeViaDefaulting(store.ToContext(ctx))
Copy link
Member

Choose a reason for hiding this comment

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

Conversion webhooks are imminent in knative/pkg btw cc @dprotaso

Copy link
Member Author

Choose a reason for hiding this comment

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

👼

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
@@ -101,7 +111,7 @@ func TestRecordPipelineRunDurationCount(t *testing.T) {
err = metrics.DurationAndCount(test.taskRun)
assertErrIsNil(err, "DurationAndCount recording recording got an error", t)
metricstest.CheckDistributionData(t, "pipelinerun_duration_seconds", test.expectedTags, 1, test.expectedDuration, test.expectedDuration)
metricstest.CheckCountData(t, "pipelinerun_count", test.expectedTags, test.expectedCount)
metricstest.CheckCountData(t, "pipelinerun_count", test.expectedCountTags, test.expectedCount)
Copy link
Member Author

Choose a reason for hiding this comment

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

@mattmoor @hrishin wasn't sure about this… I don't fully understand the behavior change there 😅

Copy link
Member

Choose a reason for hiding this comment

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

This was a while ago, but I think maybe done by @anniefu?

Copy link

Choose a reason for hiding this comment

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

Hmm, I created the metricstest helper package a while ago, but didn't do this particular test change.

A cursory glance implies that someone added a new tag to the "pipelinerun_count" metric so it has a different set of metrics tags as the "pipelinerun_duration_seconds" metric now, which isn't a problem.

@vdemeester vdemeester force-pushed the bump-k8s branch 2 times, most recently from f117375 to f416057 Compare January 29, 2020 15:40
@vdemeester
Copy link
Member Author

I think it's gonna be green 🎉 💚
@mattmoor if you have a little bit of time to review the changes 👼 😝

@mattmoor
Copy link
Member

Have a parent/teacher thing now, but will TAL after. Thanks for doing this!!! 🙏👍

@bobcatfish
Copy link
Collaborator

Just double checking, but knative is also requiring 1.15 or greater now? I think the GKE default is still 1.13 but 1.15 is available 🤔

Anyway assuming that:
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2020
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2020
@vdemeester
Copy link
Member Author

/retest

@vdemeester vdemeester force-pushed the bump-k8s branch 2 times, most recently from 8f34f71 to 61a5a52 Compare January 31, 2020 12:08
@vdemeester
Copy link
Member Author

/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 Jan 31, 2020
@vdemeester vdemeester force-pushed the bump-k8s branch 2 times, most recently from 6d9b378 to a365c27 Compare January 31, 2020 13:14
This is way simpler to follow. This change the following:
- Add validating and mutating webhook definition *in* yaml (so it gets
  registered before the webhook starts)
- Rename the controller service/deployment from `tekton-pipelines-controller` to `controller`
- Rename the webhook service/deployment from `tekton-pipelines-webhook` to `webhook`
- Use `pipeline.tekton.dev/release` instead of `tekton.dev/release` ;
  mainly to let other project use their own (`trigger.tekton.dev/release`).

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
@vdemeester
Copy link
Member Author

It is green 🎉 🕺

@ghost
Copy link

ghost commented Jan 31, 2020

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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 Jan 31, 2020
@hrishin
Copy link
Member

hrishin commented Feb 3, 2020

/LGTM

@bobcatfish
Copy link
Collaborator

@bobcatfish
Copy link
Collaborator

And the release notes too, ill add something

bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Feb 14, 2020
In tektoncd#1894 we bumped our k8s
dependencies such that a cluster must now be running at least 1.15.
tekton-robot pushed a commit that referenced this pull request Feb 14, 2020
In #1894 we bumped our k8s
dependencies such that a cluster must now be running at least 1.15.
@mattmoor
Copy link
Member

It is probably useful to also formulate some guidance around how these versions are bumped. We recently went through this exercise, and I'm happy to share if it's something you think it'd be worth aligning on.

vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this pull request Mar 10, 2020
In tektoncd#1894 we bumped our k8s dependencies such that a cluster must now
be running at least 1.15. This adds it to the README.md, to make sure
user know it too.

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
tekton-robot pushed a commit that referenced this pull request Mar 10, 2020
In #1894 we bumped our k8s dependencies such that a cluster must now
be running at least 1.15. This adds it to the README.md, to make sure
user know it too.

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
tekton-robot pushed a commit that referenced this pull request Mar 10, 2020
In #1894 we bumped our k8s dependencies such that a cluster must now
be running at least 1.15. This adds it to the README.md, to make sure
user know it too.

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
(cherry picked from commit cd981b9)
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 lgtm 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.

Make sure the webhook is always registered before allowing user to create resources
8 participants