Skip to content

Bump operator-sdk to v0.18, k8s to 1.18 #462

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 25, 2020
Merged

Bump operator-sdk to v0.18, k8s to 1.18 #462

merged 6 commits into from
Nov 25, 2020

Conversation

adambkaplan
Copy link
Member

@adambkaplan adambkaplan commented Nov 3, 2020 •
edited by qu1queee
Loading

Fixes #431 and #430

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 3, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2020
@adambkaplan adambkaplan mentioned this pull request Nov 6, 2020
Closed
@qu1queee qu1queee mentioned this pull request Nov 10, 2020
Closed
@qu1queee
Copy link
Contributor

I finally manage to reach this one, taking a look.

@adambkaplan adambkaplan changed the title WIP - operator-sdk v018 Bump operator-sdk to v0.18, k8s to 1.18 Nov 12, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2020
@qu1queee qu1queee self-requested a review November 12, 2020 13:26
@qu1queee
Copy link
Contributor

qu1queee commented Nov 12, 2020 •
edited
Loading

There is something weird here with the modules management(dependencies conflict in my opinion).

  • We are bumping to operator-sdk v0.18.2 which uses controller-runtime v0.6.0, and both are using kubernetes v0.18.2, but in the PR we are explicitly bumping to kubernetes v0.18.10.
  • We are bumping controller runtime to v0.6.1 which is using a kubernetes v0.18.4, while we explicitly bump to kubernetes v0.18.10.

I think the operator-sdk version is too old, we should do a bump to the v1.0.0, which stills kubernetes v0.18.* , but it have breaking changes in code, which we need to address anyways.

I´m trying to understand how to fix this, but leaving a comment on my observations while is almost end of my day here.

@adambkaplan
Copy link
Member Author

We are bumping to operator-sdk v0.18.2 which uses controller-runtime v0.6.0, and both are using kubernetes v0.18.2, but in the PR we are explicitly bumping to kubernetes v0.18.10.

I purposefully bumped k8s to the latest 1.18 z-stream. This ensures we get the latest bug fixes (some of which include CVEs). Same deal with controller-runtime - I explicitly bumped to the latest z-stream.

I think the operator-sdk version is too old, we should do a bump to the v1.0.0, which stills kubernetes v0.18.* , but it have breaking changes in code, which we need to address anyways.

The Operator SDK docs have explicit guidance on how to migrate from v0.17 to v0.18, v0.19, and v1. If we did this all at once, it would be very hard to determine which change led to flakes/failures.

https://sdk.operatorframework.io/docs/upgrading-sdk-version/v0.18.0/

@qu1queee
Copy link
Contributor

qu1queee commented Nov 13, 2020 •
edited
Loading

I purposefully bumped k8s to the latest 1.18 z-stream. This ensures we get the latest bug fixes (some of which include CVEs). Same deal with controller-runtime - I explicitly bumped to the latest z-stream.

Im not sure this is ideal, if controller runtime is v0.6.0 then all the k8s.io dependencies should inline with what they have in controller-runtime modules .

This is how the sdk itself does it, see sdk go modules and also mentioned on https://sdk.operatorframework.io/docs/upgrading-sdk-version/v0.18.0/#modules.

Same case for the client-go, you stick with the mapping version for the k8s you are defining in the k8s.io. Trying now to understand if everything can stay at a v0.18.2 rather than the v0.18.10

qu1queee
qu1queee reviewed Nov 13, 2020
View reviewed changes
go.mod
@@ -13,7 +13,7 @@ require (
github.com/prometheus/client_golang v1.6.0
github.com/prometheus/client_model v0.2.0
github.com/spf13/pflag v1.0.5
github.com/tektoncd/pipeline v0.17.1
github.com/tektoncd/pipeline v0.17.1-0.20201006183654-d5df1c164a48
Copy link
Contributor

Choose a reason for hiding this comment

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

This took me a little time to understand, is so weird what Tekton did, the d5df1c16 commit was merged since v0.17.1 tag but only available starting at v0.18.0 tag. I think by doing this fixed commit we only lose like 3 commits from the original v0.17.1 tag which are just around docs. I think once this PR is merged, I will bump tekton to v0.18.0 asap

@qu1queee
Copy link
Contributor

@adambkaplan dropped you a PR into this PR, pls take a look adambkaplan#1

This was referenced Nov 23, 2020
Closed
Closed
@adambkaplan
bump(*):
b47230f 
- Make go 1.14 explicit
- Bump operator-framework/operator-sdk to v0.18.2
- Bump k8s.io/* to v0.18.10
- Replace for github.com/Sirupsen/logrus (renamed)
- Replace for github.com/docker/distribution (fix version tag resolution)
- Pin docker/docker to specific version
- Update knative.dev/pkg to release-0.18
- Update tekton to d5df1c164a48 to get k8s 1.18 clientsets
- go mod tidy && go mod vendor
@adambkaplan
make generate
8148e51 
- Generate new clientsets. Context introduced in v0.18
- Generate new fakes
@adambkaplan
update integration tests
ff935ee 
- Add context to all client calls
- Add additional client options
@adambkaplan
Update operator-sdk to v0.18.2
1ba8734 
- Re-generate crds.
- Replace --local flag with local command.
@adambkaplan
Update kind
8f54809 
- Use kind v0.8.0 and k8s 1.18.2
- Run kind registry on its own network, fix default name
@adambkaplan
tekton bump changes
7febc70 
- Update taskruns wrapper
@zhangtbj
Copy link
Contributor

Thanks a lot for the version bump, Adam and Enrique.

I finally double check the PR again.
There are few script setup update and test modification. Most of the changes are related with the bump in vendor folder.
The Travis also pass.

I am going to approve and merge this PR and continue other related test based on that.

Thanks a lot!

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhangtbj

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2020
@zhangtbj zhangtbj removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2020
@openshift-merge-robot openshift-merge-robot merged commit 6e81ae8 into shipwright-io:master Nov 25, 2020
@zhangtbj zhangtbj added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2020
This was referenced Nov 25, 2020
Closed
Closed
@adambkaplan adambkaplan mentioned this pull request Dec 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qu1queee qu1queee qu1queee left review comments

@otaviof otaviof Awaiting requested review from otaviof

@zhangtbj zhangtbj Awaiting requested review from zhangtbj

Assignees

@zhangtbj zhangtbj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bump operator-sdk
5 participants
@adambkaplan @qu1queee @zhangtbj @openshift-ci-robot @openshift-merge-robot