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 Knative, Tekton and K8s imports #1209

Closed
wants to merge 1 commit into from

Conversation

mattmoor
Copy link
Member

/kind cleanup

This is WIP until the change to pipeline lands, at which point I'll update here. I'm staging this to let CI do the heavy lifting for me.

cc @piyush-garg

Submitter Checklist

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

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 29, 2021
@tekton-robot tekton-robot requested review from dibyom and wlynch August 29, 2021 20:44
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dlorenc
You can assign the PR to them by writing /assign @dlorenc in a comment when ready.

The full list of commands accepted by this bot can be found 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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 29, 2021
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/controller.go 71.1% 71.7% 0.6

@mattmoor
Copy link
Member Author

The pipeline PR this is pulling in: tektoncd/pipeline#4198

This should subsume: #1196 as well, I'll coordinate with @piyush-garg to make sure I didn't miss anything.

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/controller.go 71.1% 71.7% 0.6

@mattmoor
Copy link
Member Author

/test pull-tekton-triggers-integration-tests

Not sure why this example would be affected, so let's see if it is a flake.

@mattmoor
Copy link
Member Author

ok, so something about the label-selector example seems busted, I'll have to debug this one.

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/controller.go 71.1% 71.7% 0.6

@mattmoor
Copy link
Member Author

Not sure if I've got things set up properly, but when I run this locally I am seeing:

{"level":"error","ts":"2021-08-29T22:56:07.221Z","logger":"eventlistener","caller":"sink/sink.go:228","msg":"could not resolve interceptor URL: url resolution failed for interceptor  with: clusterinterceptor.triggers.tekton.dev \"\" not found","knative.dev/controller":"eventlistener","eventlistener":"label-selector-listener","namespace":"default","eventlistenerUID":"3a1dd92d-f482-46b6-80e7-c01261fd5ab2","/triggers-eventid":"1cc871a9-b36c-4489-87f3-19f2a3d78421","/trigger":"matching-label-trigger","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:228\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func4\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:175"}

@mattmoor
Copy link
Member Author

Ok, after a bunch of debugging I finally figured out that it was that the webhook was AWOL because of some logic in the knative/pkg bump that expanded the permissions needed to include Get on system.Namespace(). Once I finally grabbed the webhook logs it was blindingly obvious, but it took a while to get me there.

Here's the same change in pipelines when I bumped things there: tektoncd/pipeline@9d74f3f#diff-d5a724e46fb38be29b6dc700b6799f79f9d9178c6729ef5172d477f022918bb2R103

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/controller.go 71.1% 71.7% 0.6

@mattmoor
Copy link
Member Author

To expand a bit... The test was failing because it relies on the defaulting webhook to convert the v1alpha1 resource into the right shape for the dispatch logic. In particular because this used github: (deprecated) instead of ref: {name: github} (new hotness) we were seeing this error.

@mattmoor mattmoor changed the title [WIP] Bump Knative, Tekton and K8s imports Bump Knative, Tekton and K8s imports Aug 30, 2021
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2021
@mattmoor
Copy link
Member Author

The Pipelines PR landed, so I updated this and removed the WIP.

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/controller.go 71.1% 71.7% 0.6

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/controller.go 71.1% 71.7% 0.6

@mattmoor
Copy link
Member Author

Going to close this and fold what it does into #1207 since the main motivation for keeping it separate was to make reviewing the patches @piyush-garg incorporated clearer.

@mattmoor mattmoor closed this Aug 30, 2021
@mattmoor mattmoor deleted the bump-pkg-pipeline branch August 30, 2021 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesnt merit a release note. 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.

2 participants