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

Update to operator-sdk 0.1.1 #82

Merged

Conversation

ironcladlou
Copy link
Contributor

@ironcladlou ironcladlou commented Dec 6, 2018

Refactor all the operator internals to use controller-runtime. This is necessary to unpin the operator from outdated kube dependencies.

The refactor should be functionally transparent.

Unfortunately, my best advice right now for reasoning through these changes is to dive into the controller-runtime docs and source.

The new OperatorManager is (I think) the key abstraction which enables our class of operator (multi-namespaces) to make use of controller-runtime. There are ongoing discussions and prototypes upstream to try and support the use case — if and when those materialize, we can revisit our architecture.

Replaces #70.

@ironcladlou
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 6, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2018
@ironcladlou
Copy link
Contributor Author

/retest

Refactor all the operator internals to use controller-runtime. This is necessary
to unpin the operator from outdated kube dependencies.

The refactor should be functionally transparent.
@ironcladlou ironcladlou changed the title Operator sdk 0.1.x forreal Update to operator-sdk 0.1.1 Dec 7, 2018
@ironcladlou
Copy link
Contributor Author

/unhold

@ironcladlou
Copy link
Contributor Author

@openshift/sig-network-edge PTAL

/cc @bparees @shawn-hurley manager.go and its usage in operator.go may be of interest

@ironcladlou ironcladlou mentioned this pull request Dec 7, 2018
@ironcladlou
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2018
Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

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

Rest of the changes LGTM.
Looks like controller-runtime pkg gives us the needed scaffolding and provides flexibility in fetching and watching needed resources.

sdk "github.com/operator-framework/operator-sdk/pkg/sdk"
k8sutil "github.com/operator-framework/operator-sdk/pkg/util/k8sutil"
sdkVersion "github.com/operator-framework/operator-sdk/version"
"github.com/operator-framework/operator-sdk/pkg/k8sutil"

Choose a reason for hiding this comment

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

Looking at the changes, operator-sdk pkg is used only here, mainly to get WATCH_NAMESPACE env.
Why not use os.LookupEnv(...) directly and get rid of operator-sdk pkg dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'd actually also consider switching to a Cobra command and consuming this from flags anyway. In either case, seems like a followup?

Choose a reason for hiding this comment

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

yes, that works

Copy link

@pravisankar pravisankar 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 Dec 7, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, pravisankar

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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.

None yet

4 participants