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 to kubernetes 1.16 #2145

Merged
merged 4 commits into from
Nov 21, 2019
Merged

Conversation

joelanford
Copy link
Member

@joelanford joelanford commented Nov 1, 2019

Description of the change:
Bumps dependencies to support Kubernetes 1.16, including latest versions of controller-runtime, and helm that support 1.16.

Motivation for the change:
To support Kubernetes 1.16 APIs

This PR is a work-in-progress while we are awaiting a stable upstream release of controller-runtime.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2019
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 1, 2019
@joelanford joelanford force-pushed the k8s-1.16 branch 3 times, most recently from 772820a to 02a4f7c Compare November 7, 2019 14:06
@camilamacedo86 camilamacedo86 added area/dependency Issues or PRs related to dependency changes kind/feature Categorizes issue or PR as related to a new feature. labels Nov 7, 2019
@joelanford
Copy link
Member Author

/test e2e-aws-helm
/test e2e-aws-ansible
/test e2e-aws-subcommand
/test e2e-aws-go

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 19, 2019
@joelanford joelanford changed the title [WIP] *: bump to kubernetes 1.16 *: bump to kubernetes 1.16 Nov 19, 2019
@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 19, 2019
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 19, 2019
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Unable to follow the quick start with.

$ operator-sdk new app-operator
INFO[0000] Creating new Go operator 'app-operator'.     
INFO[0000] Created go.mod                               
INFO[0000] Created tools.go                         
....
INFO[0000] Validating project   
go: extracting github.com/emicklei/go-restful v2.9.5+incompatible
build app-operator/cmd/manager: cannot load github.com/Azure/go-autorest/autorest: ambiguous import: found github.com/Azure/go-autorest/autorest in multiple modules:
	github.com/Azure/go-autorest v11.1.2+incompatible (/Users/camilamacedo/go/pkg/mod/github.com/!azure/go-autorest@v11.1.2+incompatible/autorest)
	github.com/Azure/go-autorest/autorest v0.9.0 (/Users/camilamacedo/go/pkg/mod/github.com/!azure/go-autorest/autorest@v0.9.0)
Error: failed to exec []string{"go", "build", "./..."}: exit status 1
Usage:
  operator-sdk new <project-name> [flags]

@joelanford
Copy link
Member Author

@camilamacedo86 Good catch! This makes me worried that our e2e's are insufficient since such a fundamental failure wasn't found. I'll fix this and take a look at that too.

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Nov 19, 2019

I do not think that we have an e2e tests to gen the quick start or Memcached. Now that we could make the generate make file target and the scripts work properly in kubebuilder I have enjoyed a lot this idea. See how it works, I think would be very nice to have the same here:

It is very helpful since we check the testdata to be sure that our change work, to review and etc ..

@joelanford
Copy link
Member Author

@camilamacedo86 We do have a go e2e that runs operator-sdk new and then copies in some templates. See this, which eventually calls this.

@estroz Are we skipping validation in the go e2e scaffolding so that we can add the replace line into go.mod before resolving dependencies. This seems related to the conversation we were having yesterday about the test-framework dependencies and how they get resolved. I'm now thinking I might have been incorrect in this comment.

Replicating the steps of the go e2e test to create the operator project without validation, adding the replace line for the local operator-sdk replacement, and then running go mod tidy and go build ./... works. I then ran go list -m all and it appears that the dependencies of the replaced module are honored. (i.e. I'm getting the kubernetes 1.16 deps from the local repo, not kubernetes 1.15 from master)

It seems like the error that @camilamacedo86 posted happens because the scaffolded go.mod file uses master, but master does not yet have the rest of the changes that are required to make the kubernetes-1.16.2 replacements work.

If I'm understanding the above correctly, once this PR is actually merged into master the autorest dependency errors will disappear.

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Nov 19, 2019

Hi @joelanford,

We have the tests but they do not work exactly the generate in Kube. Actually, I have been thinking if we could not rip all and does as kube to gen the testdata and use it to do the tests.

It seems like the error that @camilamacedo86 posted happens because the scaffolded go.mod file uses master, but master does not yet have the rest of the changes that are required to make the kubernetes-1.16.2 replacements work.

I am not sure if I got this point. The test was made with this PR (get the pr, make install then follow the quick start), then the scaffold was gen with its changes in the internal/scaffold/ansible/go_mod.go. I understand that when the PR is 100% it will works.

@estroz
Copy link
Member

estroz commented Nov 19, 2019

@joelanford yes that's why we skip validation.

@camilamacedo86 the issue you saw only surfaces when using a k8s bump branch before its merge with the remote master occurs. The master branch version that the scaffolded go.mod pulls in does not have the updates from the branch you built yet. Those changes are only present on @joelanford's k8s-1.16 branch. What the e2e tests do is equivalent to adding:

replace github.com/operator-framework/operator-sdk => github.com/joelanford/operator-sdk k8s-1.16

To the go.mod file.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2019
@joelanford
Copy link
Member Author

@camilamacedo86 Does @estroz's comment make sense to you? What I've gleaned by testing this is that it is not valid to test a project created from a branch of operator-sdk without adding the replace line, because the project's go.mod file doesn't know anything about the branch without the replace.

Therefore when testing the scaffolded projects that use go.mod with any branch, it is always necessary to use replace in the go.mod file to force it to use the branch.

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm /approved

Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

Although we should update the CHANGELOG as well to point out the bump to k8s 1.16 and controller-runtime v0.4.0

@joelanford
Copy link
Member Author

@hasbro17 Yep. I somehow always forget to do that. Thanks for the constant reminders 🙂

joelanford added a commit to joelanford/operator-sdk that referenced this pull request Nov 20, 2019
…dition of Deprecated header in 0.12.0 release section
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 20, 2019
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -13,8 +13,8 @@ cd test/test-framework

# Ensure test-framework is up-to-date with current Go project dependencies.
# NOTE: the SDK dependency version still needs updating on a new release.
echo "$(../../build/operator-sdk print-deps)" > go.mod
Copy link
Member

@estroz estroz Nov 20, 2019

Choose a reason for hiding this comment

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

Any reason for this change? The reason I had sdk_version=... above this command is we want to save the current version and add it back to go.mod later. Otherwise version in require github.com/operator-framework/operator-sdk ${version} will be updated every time a new commit is made in the master branch since go list -m will resolve require github.com/operator-framework/operator-sdk master.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the commit prior to this change, the gen-test-framework.sh script is failing for me. I get this in the test/test-framework dir:

$ go list -m -f '{{.Version}}' github.com/operator-framework/operator-sdk
go: github.com/operator-framework/operator-sdk@v0.12.1-0.20191114194531-79e6369540c6 requires
	k8s.io/kubectl@v0.0.0: reading k8s.io/kubectl/go.mod at revision v0.0.0: unknown revision v0.0.0

But you're right. If this gets committed, it seems like every PR will have to regenerate the test framework, which would be a non-starter.

The above error happens because we use replace to pull in operator-sdk at a different version than what the existing test-framework's go.mod file is compatible with. More specifically, the SDK's go.mod file that has been updated for k8s-1.16 has a require line for k8s.io/kubectl v0.0.0 and a replace line to fill in the correct version. But since replace lines are only honored in the main module (not dependent modules), and the test-framework doesn't have a replace for k8s.io/kubectl, we have a problem.

To fix this, I think we can just hardcode the operator-sdk require line to v0.0.0 since we always use a replace line.

@joelanford joelanford merged commit 04629a2 into operator-framework:master Nov 21, 2019
@joelanford joelanford deleted the k8s-1.16 branch November 21, 2019 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependency Issues or PRs related to dependency changes kind/feature Categorizes issue or PR as related to a new feature. 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.

5 participants