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.

Sign up for GitHub

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

Jump to bottom

make registry installation a component #18956

Merged
merged 2 commits into from
Mar 23, 2018
Merged

make registry installation a component #18956

merged 2 commits into from
Mar 23, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 12, 2018

Demonstrate a simple way of making a component and installing it. This builds on previous pulls and shows how we can start define an interface.

I think after we switch pieces over, we'll find points of commonality as their entry points. I suspect they will include:

  1. cluster-admin.kubeconfig
  2. docker helper? We should try to switch this to actually installing with pods.
  3. uninstall API. Something to remove it anyway.
  4. idempotent. We will call it on every cluster up

/assign @mfojtik
/assign @soltysh

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2018
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 12, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Mar 12, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Mar 13, 2018

/retest

soltysh
soltysh approved these changes Mar 15, 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.

One question and one nit about upstream pr. The rest lgtm.

vendor/k8s.io/kubernetes/staging/src/k8s.io/client-go/tools/clientcmd/client_config.go Outdated
@@ -100,6 +100,18 @@ func NewInteractiveClientConfig(config clientcmdapi.Config, contextName string,
return &DirectClientConfig{config, contextName, overrides, fallbackReader, configAccess, promptedCredentials{}}
}

func NewClientConfig(config clientcmdapi.Config) ClientConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

How were you able to pass commit validation without an UPSTREAM commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How were you able to pass commit validation without an UPSTREAM commit?

uh, not sure. I'll update the pull though.

pkg/oc/bootstrap/clusterup/components/registry/registry_install.go Outdated
RegistryServiceIP = "172.30.1.1"
)

type RegistryComponentOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you thinking about creating an interface that all the 'install plugins' will re-use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you thinking about creating an interface that all the 'install plugins' will re-use?

Yes, but I want to see what I need before I commit. I think I will end up passing more information into the install method.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot of code for something that will eventually be using an operator. What’s the short term benefit to formalizing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a lot of code for something that will eventually be using an operator. What’s the short term benefit to formalizing this?

I get to formalize input and output interfaces for different components that want to participate in cluster up. I also get create oc cluster add/remove <component> and that lets me bound the feature creep of cluster up.

Copy link
Contributor Author

@deads2k deads2k Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is a lot of code for something that will eventually be using an operator. What’s the short term benefit to formalizing this?

Also, most of this code is a move with more explicit input to help us see the true dependency structures we need to snip.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, I think this formalization will help is see the dependency chain better and we can promote this to operators when we will be ready... it also helps clean up the cluster up code we have today.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 16, 2018
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Mar 16, 2018

/retest

3 similar comments
@deads2k
Copy link
Contributor Author

deads2k commented Mar 16, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 17, 2018

/retest

@soltysh
Copy link
Contributor

soltysh commented Mar 19, 2018

/retest

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2018
@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 Mar 20, 2018
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2018
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2018
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Mar 21, 2018

/refresh

@soltysh
Copy link
Contributor

soltysh commented Mar 21, 2018

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Mar 21, 2018

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 22, 2018 •
edited
Loading

@deads2k: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_networking_minimal 1dc3faf link /test extended_networking_minimal

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.

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.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

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

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit eb016b3 into openshift:master Mar 23, 2018
@deads2k deads2k deleted the up-21-componentinstall branch July 3, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@mfojtik mfojtik mfojtik left review comments

@soltysh soltysh soltysh approved these changes

@coreydaley coreydaley Awaiting requested review from coreydaley

@jim-minter jim-minter Awaiting requested review from jim-minter

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/L Denotes a PR that changes 100-499 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.
7 participants
@deads2k @openshift-ci-robot @soltysh @openshift-bot @openshift-merge-robot @mfojtik @smarterclayton