Skip to content

Conversation

@camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented May 5, 2019

Description of the change:

Changed

  • Upgrade the version of the dependency controller-runtime from v0.1.10 to v0.2.0-beta.1
  • Upgrade Kubernetes dependency versions from kubernetes-1.13.1 to kubernetes-1.14.1
  • Upgrade github.com/operator-framework/operator-lifecycle-manager dependency from the commit b8a4faf68e36feb6d99a6aec623b405e587b17b1 to tag 0.10.1
  • Upgrade k8s.io/kube-openapi from 0cf8f7e6ed1d2e3d47d02e3b6e559369af24d8 dependency to a 01b7d5d6c2258c80a4a10070f3dee9cd575d9c7
  • Upgrade others menor versions.

Motivation for the change:
Address the commit kubernetes-sigs/controller-runtime@fc804a4

         // Create a new Cmd to provide shared dependencies and start components
	mgr, err := manager.New(cfg, manager.Options{
		NewCache: cache.MultiNamespacedCacheBuilder(namespaces),
	})

Progress

Working on in Rebase/Merge the changes/fixes from @estroz #1512

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 5, 2019
@openshift-ci-robot
Copy link

Hi @camilamacedo86. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 5, 2019
@camilamacedo86 camilamacedo86 changed the title Upgrade controller Upgrade the version of the dependency [controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) from v0.1.10 to v0.2.0-alpha.0 May 5, 2019
@camilamacedo86 camilamacedo86 changed the title Upgrade the version of the dependency [controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) from v0.1.10 to v0.2.0-alpha.0 Upgrade the version of the dependency controller-runtime from v0.1.10 to v0.2.0-alpha.0 May 5, 2019
@openshift-ci-robot
Copy link

@camilamacedo86: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Hey @camilamacedo86 👋 there is already an open PR to migrate to that release of controller-runtim #1369 so maybe we can sync there to see the difference between the work and maybe somehow combine it? :)

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented May 6, 2019

Hi @lilic,

I checked the PR #1369 made by @corinnekrych. This PR shows missing changes and because of it is not passing the ansible as well. Also, she increases the version of Kube.

This one here, shows be working very well but the go test is facing timeout which I am not sure if is related or not to the changes made here. @lilic

@corinnekrych let's combine the forces? I will try to check the changes made by you and apply here too and I'll ping you tomorrow for we check how we can move together. :-)

NOTE:
@lilic @corinnekrych
I spoke with @shawn Hurley and:

  1. We can leave it working 100% but shows that we should wait for the release of the stable version which should occur this week
  2. Should be better we wait until (Wed) to merge it anyway because of the OCP 4.1 code freeze

@estroz
Copy link
Member

estroz commented May 6, 2019

I'd rather wait for a v0.2.0 stable release. There's no urgency to upgrade for the next release.

@lilic
Copy link
Member

lilic commented May 6, 2019

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 6, 2019
@lilic
Copy link
Member

lilic commented May 6, 2019

Unfortunately for us has urgency since we are stuck for not being able to attend a requirement in our projects which are using it.

Sorry to hear that, but we can't have a nonstable release of such a huge dependency in our project, as we do promise certain stability to our users. @camilamacedo86 we discussed this and we want to wait until Wednesday at least, we will know when the stable controller-runtime release will happen and we will get back to you then. But we would be happy to have this contribution!

@lilic
Copy link
Member

lilic commented May 6, 2019

A couple of things before this is ready to be merged:

  • stable release of controller-runtime
  • migration document as it has a lot of breaking changes for users using current 0.7.0 release
  • we might want to release one more time before this gets merged

cc @joelanford @theishshah @shawn-hurley @hasbro17 @AlexNPavel

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 7, 2019
@camilamacedo86
Copy link
Contributor Author

Hi @lilic,

Now all shows working fine here.

The changes that need to be done by in the user's project from the latest version to this one is in the section break changes in the Changelog. See def0660

You added migration document as it has a lot of breaking changes for users using current 0.7.0 release , however, for the projects which are using 0.7.0(latest) to this one are not too many changes and the current migration doc is not covering from 0.1.0 to 0.7.0 either. So, shows that do the migration doc from 0.1.0 to 0.7.0 is not part of this task. Could we leave it to be done in another PR as part of another task?

@lilic
Copy link
Member

lilic commented May 9, 2019

@camilamacedo86 Yes we have an issue open to update the migration guide, we put breaking changes in the CHANGELOG. And besides new files and commands added, there were not many major changes since 0.1.0 onwards but yes we need to update that in the near future.

We can help with adding the migration guide no problem regarding that, but we agreed to not merge this until we add that to the PR cc @hasbro17 ^

The new stable release of controller-runtime will be before kubecon which is in 10 days time or so, according to yesterdays SIG meeting, so we should be able to merge that then. If this is delaying your progress on the operator you can create a branch on your fork with these changes and vendor that instead? Thanks again for this!

cc @joelanford @shawn-hurley @theishshah @AlexNPavel @estroz Thoughts? ^

@camilamacedo86
Copy link
Contributor Author

Hi @lilic,

Really tks for all your support and help with this subject. No problem at all. Let's waiting for the release 👍

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2019
@lilic
Copy link
Member

lilic commented May 17, 2019

@camilamacedo86 FYI there was a new release. I would wait until next week before updating this PR, as I suspect there will be some more changes, but I believe that the maintainer wants to release it for kubecon which is next week. Thanks so much for working on this! https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.2.0-beta.0

@camilamacedo86 camilamacedo86 changed the title Upgrade the version of the dependency controller-runtime from v0.1.10 to v0.2.0-alpha.0 Upgrade the version of the dependency controller-runtime from v0.1.10 to v0.2.0-beta.1 May 23, 2019
@yanirq
Copy link

yanirq commented May 24, 2019

Hi @camilamacedo86
I am also looking into what is the operator-sdk status with :

Allow users to watch/cache N namespaces as follows.

From the operator-sdk point of view I am missing here the part of rbac roles that are created by the operator-sdk for watched namespaces.
Maybe this is out of this PR's scope, but what is the behaviour or permissions that should be generated/manually edited for the operator when handling N namespaces ?
Would it be mandatory to configure the operator only as cluster-scoped ?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2019
@lilic
Copy link
Member

lilic commented May 29, 2019

@camilamacedo86 Thanks for your hard work on this! I will have a test and play around with this today, and maybe we can start working on the migration guide. cc @joelanford

In the mean time you have some failing tests, I will retest as the error is a bit odd.

/retest

@lilic
Copy link
Member

lilic commented May 29, 2019

/retest

@estroz
Copy link
Member

estroz commented May 30, 2019

It looks like we won't be able to upgrade to v0.2.0 until OLM upgrades to kubernetes-1.14.1. I did some testing with these changes and saw the following errors (after fixing a few caused by the updates here):

$ dep ensure
$ make install
# github.com/operator-framework/operator-sdk/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient
vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/serviceaccount.go:38:2: not enough arguments to return
vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/serviceaccount.go:38:10: c.Core undefined (type *Client has no field or method Core)
make: *** [Makefile:46: install] Error 2

This can be fixed by using c.CoreV1() instead of a full kubernetes-1.14.1 bump, but we still need to get that merged before this works. I'd rather see a bump than this OLM patch anyways.

@estroz
Copy link
Member

estroz commented May 31, 2019

OLM PR to bump kube deps: operator-framework/operator-lifecycle-manager#864

Edit: this has been merged.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2019
@estroz
Copy link
Member

estroz commented Jun 6, 2019

@camilamacedo86 #1512 has all the necessary code changes for the bump. I recommend merging https://github.com/estroz/operator-sdk/tree/bump-controller-runtime-v0.2.0 with your branch and pushing to this PR. Otherwise I can do the reverse with my branch and make #1512 the active bump PR.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2019
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.

Quite a few changes still need addressing on top of those from comments here. See #1512.

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Jun 6, 2019

Hi @estroz

Really thank you for your help and support here. 👍

I rebase the commits in order to make easier we review and go forward as well. Also, I addressed all comments raised by you and I am working on in adding your changes and fixes from #1512 here. I understand that we need both, however, a direct merge/rebase will not work yet I will be solving/checking asap.

@estroz
Copy link
Member

estroz commented Jun 7, 2019

@camilamacedo86 #1512 already passes CI, so we should defer to that as the main bump PR. Thanks again for submitting this PR! We appreciate the urgency, as this is a big but necessary change.

@estroz estroz closed this Jun 7, 2019
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Jun 7, 2019

Hi @estroz if you could add there what was made here as well is great !!.
I had a few issues with the merge and I was fixing.

@camilamacedo86 camilamacedo86 deleted the UPGRADE-CONTROLLER branch June 7, 2019 20:24
@openshift-ci-robot
Copy link

@camilamacedo86: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/unit d41ad22 link /test unit
ci/prow/sanity d41ad22 link /test sanity

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

6 participants