Skip to content

add wiring for running bare kube-controller-manager#18100

Merged
openshift-merge-robot merged 3 commits intoopenshift:masterfrom
deads2k:controller-19-flaggable
Jan 17, 2018
Merged

add wiring for running bare kube-controller-manager#18100
openshift-merge-robot merged 3 commits intoopenshift:masterfrom
deads2k:controller-19-flaggable

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Jan 12, 2018

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 12, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Jan 12, 2018
smarterclayton
smarterclayton reviewed Jan 12, 2018
View reviewed changes
vendor/k8s.io/kubernetes/cmd/kube-controller-manager/app/options/options.go

// AddFlags adds flags for a specific CMServer to the specified FlagSet
func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabledByDefaultControllers []string) {
fs.StringVar(&s.OpenShiftConfig, "openshift-config", s.OpenShiftConfig, "indicates that this process should be compatible with openshift start master")
Copy link
Contributor

@smarterclayton smarterclayton Jan 12, 2018

Choose a reason for hiding this comment

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

We might want to make this a hidden flag

Copy link
Contributor Author

@deads2k deads2k Jan 12, 2018

Choose a reason for hiding this comment

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

We might want to make this a hidden flag

Ok.

@deads2k deads2k force-pushed the controller-19-flaggable branch from 508734e to 91994d1 Compare January 15, 2018 18:52
@deads2k deads2k changed the title [POC] add wiring for running bare kube-controller-manager add wiring for running bare kube-controller-manager Jan 15, 2018
deads2k
deads2k commented Jan 15, 2018
View reviewed changes
vendor/k8s.io/kubernetes/cmd/kube-controller-manager/app/patch.go
return func() {}, nil
}

// TODO this gets removed when no longer take flags and no longer build a recycler template
Copy link
Contributor Author

@deads2k deads2k Jan 15, 2018

Choose a reason for hiding this comment

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

I've re-organized a bit and added comments about how we can get rid of each of these patches.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 15, 2018

/assign mfojtik
/assign soltysh

I've updated this to be cleaner and have criteria for eliminating the patches we're carrying. I think its ready to push forward.

@deads2k deads2k force-pushed the controller-19-flaggable branch from 58b8c50 to 3f09eba Compare January 15, 2018 19:31
@deads2k deads2k mentioned this pull request Jan 15, 2018
Closed
@soltysh
Copy link
Contributor

soltysh commented Jan 16, 2018

David gave me some more details about this PR on irc.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 16, 2018

/hold

I just want to make sure that @smarterclayton is ready to push on this approach. All the options are bad.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2018
@deads2k deads2k force-pushed the controller-19-flaggable branch from 3f09eba to f829b1a Compare January 16, 2018 16:24
@deads2k deads2k added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 16, 2018
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 16, 2018

This doesn't force us to use it, but does organize our patches to make it clear what we're doing

/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 Jan 16, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 16, 2018

@soltysh I added another commit to wire up usage in openshift to avoid duplicate methods. Take another look?

@deads2k deads2k force-pushed the controller-19-flaggable branch from 79dd851 to 8d3b3c8 Compare January 16, 2018 17:00
@deads2k
Copy link
Contributor Author

deads2k commented Jan 16, 2018

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Jan 16, 2018

/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2018
soltysh
soltysh approved these changes Jan 16, 2018
View reviewed changes
Copy link
Contributor

@soltysh soltysh 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
Copy link

openshift-ci-robot commented Jan 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, soltysh

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@soltysh
Copy link
Contributor

soltysh commented Jan 16, 2018

/retest

@deads2k deads2k added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 16, 2018
@deads2k deads2k force-pushed the controller-19-flaggable branch from 8d3b3c8 to bd48fc8 Compare January 16, 2018 21:22
@deads2k
Copy link
Contributor Author

deads2k commented Jan 16, 2018

/retest

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Jan 17, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Jan 17, 2018

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 363a5f4 into openshift:master Jan 17, 2018
@deads2k deads2k deleted the controller-19-flaggable branch January 24, 2018 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adelton adelton Awaiting requested review from adelton

@liggitt liggitt Awaiting requested review from liggitt

2 more reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@soltysh soltysh soltysh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mfojtik mfojtik

@soltysh soltysh

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. vendor-update Touching vendor dir or related files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@deads2k @soltysh @openshift-ci-robot @openshift-merge-robot @smarterclayton @mfojtik