Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bump Pipeline, k8s, and Knative dependencies #1353

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

abayer
Copy link
Contributor

@abayer abayer commented Apr 27, 2022

Changes

This change uses non-tagged commits from knative.dev/serving and knative.dev/eventing because
their v0.31.0 releases require a minimum k8s cluster version of 1.22, and I wanted to avoid
requiring that if at all possible. Instead, we depend on the latest commit in each before they
changed to depending on a knative.dev/pkg commit newer than the one Pipeline depends on.
Also add a Makefile, sync .golangci.yml with Pipeline's (other than requiring comments for
exported functions/types/etc, which I'll come back and do in a followup), and fix some new-as-a-result
linter issues.

The initial motivation was to get k8s and Knative dependencies bumped so I can work on bumping Pipeline in the CLI to make changes to support the new minimal embedded status functionality, but while I was here... =)

/kind misc

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. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Apr 27, 2022
@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

/assign @dibyom

@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 27, 2022
@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

Whoops, missed an update-codegen.sh run!

@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

/retest

Huh, no, I didn't - it doesn't change anything when I run it locally. Not sure why the build tests are failing...

@abayer abayer force-pushed the pipeline-0.35.0-k8s-knative branch from a495ebb to b6d9073 Compare April 27, 2022 13:45
@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

That's just weird - update-codegen.sh doesn't do anything on my laptop, but does on my desktop. I am very confused, but am updating with the output on my desktop.

@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

/retest

And now the controller and webhook pods are barfing on the integration tests, but not in kind locally for me. Hrrrm.

@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

Ok! For some reason that I don't yet understand, Pipeline (depending on k8s 1.23) runs just fine in a k8s 1.21 cluster, but Triggers in this PR (also depending on 1.23?) does not, with

{"level":"fatal","ts":"2022-04-27T14:18:48.931Z","logger":"controller","caller":"sharedmain/main.go:313","msg":"Version check failed","error":"kubernetes version \"1.21.10\" is not compatible, need at least \"1.22.0-0\" (this can be overridden with the env var \"KUBERNETES_MIN_VERSION\")","stacktrace":"knative.dev/pkg/injection/sharedmain.CheckK8sClientMinimumVersionOrDie\n\tknative.dev/pkg@v0.0.0-20220412134708-e325df66cb51/injection/sharedmain/main.go:313\nknative.dev/pkg/injection/sharedmain.MainWithConfig\n\tknative.dev/pkg@v0.0.0-20220412134708-e325df66cb51/injection/sharedmain/main.go:215\nmain.main\n\tgithub.heygears.com/tektoncd/triggers/cmd/controller/main.go:92\nruntime.main\n\truntime/proc.go:255"}

Wonder if this is a side effect of having to bump the knative.dev/serving and knative.dev/eventing versions to get versions depending on knative.dev/pkg greater than or equal to the one Pipeline depends on, v0.0.0-20220329144915-0a1ec2e0d46c. Probably!

@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

Ok, yeah, it's knative/pkg@4d62e1d. Pipeline depends on knative/pkg@0a1ec2e0d46c, but there's no serving or eventing releases which depend on a knative.dev/pkg commit between the one Pipeline depends on and the one bumping min k8s to 1.22.

@abayer abayer force-pushed the pipeline-0.35.0-k8s-knative branch from b6d9073 to 8a6a114 Compare April 27, 2022 14:42
@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

Ok, I switched knative.dev/serving and knative.dev/eventing to v0.30.2, the last release before knative/pkg@4d62e1d, and then bumped knative.dev/pkg to knative/pkg@0a1ec2e0d46c, the one Pipeline depends on, and things...seem to work on k8s 1.21? But I'd definitely appreciate input from @mattmoor or someone else who actually knows what we're doing with our knative deps. =)

@abayer abayer force-pushed the pipeline-0.35.0-k8s-knative branch from 8a6a114 to 84d651b Compare April 27, 2022 14:45
@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

Nope, I tell a lie - knative.dev/serving v0.30.2 doesn't compile against k8s 1.23. Siiiigh.

This change uses non-tagged commits from `knative.dev/serving` and `knative.dev/eventing` because
their `v0.31.0` releases require a minimum k8s cluster version of 1.22, and I wanted to avoid
requiring that if at all possible. Instead, we depend on the latest commit in each before they
changed to depending on a `knative.dev/pkg` commit newer than the one Pipeline depends on.

Also add a Makefile, sync `.golangci.yml` with Pipeline's (other than requiring comments for
exported functions/types/etc, which I'll come back and do in a followup), and fix some new-as-a-result
linter issues.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer abayer force-pushed the pipeline-0.35.0-k8s-knative branch from 84d651b to a1f725f Compare April 27, 2022 15:13
@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

/hold

Ok, so to go to releases of knative.dev/serving and knative.dev/eventing which themselves depend on k8s 1.23, like Pipeline does, we'd also have to bump our minimum k8s cluster version to 1.22. Therefore I decided to depend on the last commit in each of knative.dev/serving and knative.dev/eventing which depended on a knative.dev/pkg commit which isn't newer than the one Pipeline v0.35.0 depends on. Which isn't ideal, obviously, but it's what I needed to do to make this work.

I put the PR on hold because...well, I'm not sure if any of this is a good idea. =)

@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 Apr 27, 2022
@dibyom dibyom mentioned this pull request Apr 27, 2022
4 tasks
@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

/retest

@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

phew - the previous failure was just transient noise.

@abayer
Copy link
Contributor Author

abayer commented Apr 27, 2022

/assign @imjasonh

Since you opened #1351. =)

@dibyom
Copy link
Member

dibyom commented Apr 27, 2022

/approve

Thanks @abayer !

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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 Apr 27, 2022
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you!

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2022
@dibyom
Copy link
Member

dibyom commented Apr 29, 2022

/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 Apr 29, 2022
@tekton-robot tekton-robot merged commit 7146397 into tektoncd:main Apr 29, 2022
@abayer
Copy link
Contributor Author

abayer commented Apr 29, 2022

yay! =)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. 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.

4 participants