Skip to content

Conversation

@estroz
Copy link
Member

@estroz estroz commented Jun 3, 2019

Description of the change: bump controller-runtime to v0.2.0 and helm to 2.14.0, both of which depend on kubernetes-1.14.x dependencies.

Motivation for the change: from #1388:

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),
  })

@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 Jun 3, 2019
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. 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 Jun 3, 2019
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/client-go/discovery/cached"
cached "k8s.io/client-go/discovery/cached"
Copy link
Member Author

Choose a reason for hiding this comment

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

The compiler throws errors if there's no build identifier.

@estroz
Copy link
Member Author

estroz commented Jun 6, 2019

@estroz
Copy link
Member Author

estroz commented Jun 6, 2019

/test marker

@lilic
Copy link
Member

lilic commented Jun 7, 2019

@camilamacedo86 @estroz Whichever PR lands in, we need a migration doc, I guess we can do this in a parallel PR for ease of reviewing?

@joelanford
Copy link
Member

Also, what did we decide on what to do with this PR? Sit on it? Merge it into a separate branch? Merge it into master? Right now looks like it's targetting master.

@estroz
Copy link
Member Author

estroz commented Jun 7, 2019

@joelanford IIRC we agreed to merge with master because the original controller-runtime refactor was a much larger change than v0.1.x -> v0.2.x.

@joelanford
Copy link
Member

@estroz Haha. I thought I heard the opposite based on the same premise. What I heard was "The separate branch for the original controller-runtime migration was a pain. Since the changes this time around are minimal relative to last time, a separate branch shouldn't be a big deal."

If we merge this to master, then we're either going to make a release on a non-stable version of controller-runtime, or we're going to be forced to wait until controller-runtime reaches a stable v0.2.0 release.

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Jun 7, 2019

Hi @estroz,

Could you add here the follow commit?

And upgrade the following deps as well?

[[constraint]]
  name = "github.com/sirupsen/logrus"
  version = "1.4.2"

[[constraint]]
  name = "github.com/spf13/cobra"
  version = "0.0.4"

Also, I think you can use the Changelog and just update the commit number c5bdaee

Really tks for the help on it 👍

@estroz
Copy link
Member Author

estroz commented Jun 7, 2019

@joelanford very possible I heard incorrectly. I'll make a refactor branch like we did last time.

@estroz estroz changed the title [WIP] (for testing purposes only) bump to controller-runtime v0.2.0 deps: bump to controller-runtime v0.2.0, kubernetes-1.14.1, and helm v2.14.1 Jun 7, 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 Jun 7, 2019
@estroz estroz requested review from joelanford and removed request for joelanford June 10, 2019 16:53
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2019
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

Not sure how to handle the difference in version numbers in the CHANGELOG since #1612 was merged. So if those suggestions don't make sense, you can ignore them. Should we merge master into refactor/controller-runtime-v0.2.0?

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 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

Just one comment on the removed replace directive for the prometheus-operator dependency in go.mod.

@estroz estroz force-pushed the bump-controller-runtime-v0.2.0 branch from 5912ff4 to 8e6dd4c Compare July 3, 2019 17:43
@estroz estroz force-pushed the bump-controller-runtime-v0.2.0 branch from 8e6dd4c to d9b89bb Compare July 3, 2019 18:06
@estroz estroz merged commit fc3b764 into operator-framework:refactor/controller-runtime-v0.2.0 Jul 3, 2019
@estroz estroz deleted the bump-controller-runtime-v0.2.0 branch July 3, 2019 23:10
ricoberger added a commit to ricoberger/vault-secrets-operator that referenced this pull request Oct 28, 2019
Update the underlying operator-sdk to v0.11.x. The new version contains
version v0.2.0 which introduces a new feature to watch multiple
namesapces. This is needed to implement #20.

For more information see operator-framework/operator-sdk#1512.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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