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

Cluster-API apiserver should be namespace-ed #509

Merged
merged 1 commit into from
Sep 26, 2018
Merged

Cluster-API apiserver should be namespace-ed #509

merged 1 commit into from
Sep 26, 2018

Conversation

ashish-amarnath
Copy link
Contributor

@ashish-amarnath ashish-amarnath commented Sep 21, 2018 •
edited by roberthbailey

  • templatize Namespace in ClusterAPIAPIServerConfigTemplate
  • pass namespace to templating
  • accept gen file edit

What this PR does / why we need it:
Identified a bug /gap in PR #481 has a gap where the cluster-api apiserver, for all clusters, would get created in the default namespace. This PR fixes that by passing the namespace to the manifest templating.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #252

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Support cluster and machine in multiple namespaces.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 21, 2018
@ashish-amarnath ashish-amarnath changed the title [WIP] Cluster-API apiserver should be namespace-ed Cluster-API apiserver should be namespace-ed Sep 21, 2018
@k8s-ci-robot k8s-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 Sep 21, 2018
@Lion-Wei
Copy link

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 21, 2018
xmudrii
xmudrii reviewed Sep 21, 2018
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

Looks good to me, great job!

I've mentioned some points where default is still being used, so make sure to check those.

pkg/deployer/clusterapiservertemplate.go Show resolved Hide resolved
pkg/deployer/clusterapiservertemplate.go Show resolved Hide resolved
pkg/deployer/clusterapiserver.go Show resolved Hide resolved
pkg/deployer/clusterapiserver.go Outdated
@@ -73,8 +73,13 @@ func getApiServerCerts() (*caCertParams, error) {
return certParams, nil
}

// GetApiServerYaml returns the clusterapi-apiserver manifest used for deployment
// GetApiServerYaml returns the cluster-apiserver manifest for deployment in the default namespace
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nitpick: I think it indeed is clusterapi-apiserver. :)

@xmudrii xmudrii mentioned this pull request Sep 21, 2018
Closed
xmudrii
xmudrii reviewed Sep 23, 2018
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment •
edited

Choose a reason for hiding this comment

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

Looks great! Left two more comments with suggestions for the potential improvements.

pkg/deployer/clusterapiserver.go Outdated Show resolved Hide resolved
pkg/deployer/clusterapiserver.go Show resolved Hide resolved
xmudrii
xmudrii approved these changes Sep 24, 2018
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

Looks great now, great job!

@xmudrii
Copy link
Member

xmudrii commented Sep 24, 2018

/lgtm
/assign @roberthbailey @detiber

@k8s-ci-robot
Copy link
Contributor

@xmudrii: changing LGTM is restricted to assignees, and only kubernetes-sigs/cluster-api repo collaborators may be assigned issues.

In response to this:

/lgtm
/assign @roberthbailey @detiber

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.

detiber
detiber reviewed Sep 24, 2018
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

Question about the diff in the generated file, otherwise lgtm.

pkg/apis/cluster/zz_generated.api.register.go Outdated
}

type NetworkRanges struct {
CIDRBlocks []string
Copy link
Member

Choose a reason for hiding this comment

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

I'm having difficulty telling if there is an actual diff here outside of the generation script re-ordering the structs. If there is no actual diff, can you replace this with the existing upstream version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you didn't change api types, there shouldn't be any relevant changes in this file. Please remove from the PR.

roberthbailey
roberthbailey reviewed Sep 25, 2018
View reviewed changes
Copy link
Contributor

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

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

This change seems fine, but I'm removing most of this code as part of #494 so don't get too dependent on it.

pkg/apis/cluster/zz_generated.api.register.go Outdated
}

type NetworkRanges struct {
CIDRBlocks []string
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you didn't change api types, there shouldn't be any relevant changes in this file. Please remove from the PR.

@ashish-amarnath
Cluster-API apiserver should be namespace-ed
8593eda 
* templatize Namespace in ClusterAPIAPIServerConfigTemplate
* pass namespace to templating
@ashish-amarnath
Copy link
Contributor Author

@detiber and @roberthbailey I have reverted the changes to the generated files.

@detiber
Copy link
Member

detiber commented Sep 25, 2018

/lgtm

@ashish-amarnath
Copy link
Contributor Author

/test

@ashish-amarnath
Copy link
Contributor Author

/retest

@detiber
Copy link
Member

detiber commented Sep 25, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2018
@roberthbailey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashish-amarnath, roberthbailey

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2018
@k8s-ci-robot k8s-ci-robot merged commit 3f53b7a into kubernetes-sigs:master Sep 26, 2018
@ashish-amarnath ashish-amarnath deleted the templatize-ns branch September 26, 2018 06:34
@xmudrii xmudrii mentioned this pull request Sep 26, 2018
Merged
@scruplelesswizard scruplelesswizard mentioned this pull request Sep 26, 2018
Merged
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@detiber detiber detiber left review comments

@roberthbailey roberthbailey roberthbailey left review comments

@xmudrii xmudrii xmudrii approved these changes

@jessicaochen jessicaochen Awaiting requested review from jessicaochen

@karan karan Awaiting requested review from karan

Assignees

@detiber detiber

@roberthbailey roberthbailey

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support cluster and machine in multiple namespaces in clusterctl create
6 participants
@ashish-amarnath @Lion-Wei @xmudrii @k8s-ci-robot @detiber @roberthbailey