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

[Federation][init-10d] Use the right service names in controller manager. #36294

Merged
merged 1 commit into from
Nov 15, 2016
Merged

[Federation][init-10d] Use the right service names in controller manager. #36294

merged 1 commit into from
Nov 15, 2016

Conversation

madhusudancs
Copy link
Contributor

@madhusudancs madhusudancs commented Nov 5, 2016 •
edited by k8s-oncall
Loading

Please review only the last commit here. This is based on PR #36048 which will be reviewed independently.

Design Doc: PR #34484

cc @kubernetes/sig-cluster-federation @nikhiljindal

This change is Reviewable

@madhusudancs madhusudancs added area/cluster-federation release-note-none Denotes a PR that doesn't merit a release note. labels Nov 5, 2016
@madhusudancs madhusudancs added this to the v1.5 milestone Nov 5, 2016
@madhusudancs madhusudancs assigned ghost Nov 5, 2016
@madhusudancs madhusudancs mentioned this pull request Nov 5, 2016
Closed
@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 5, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 3fa94ff. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2016
ghost
ghost reviewed Nov 7, 2016
View reviewed changes
federation/pkg/kubefed/init/init.go
"--kubeconfig=/etc/federation/controller-manager/kubeconfig",
"--dns-provider=gce",
"--dns-provider=google-clouddns",
Copy link

Choose a reason for hiding this comment

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

This needs to work for non-Google cloud providers? e.g. How do I specify that I want to use AWS Route53?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quinton-hoole @irfanurrehman fixed this in PR #36492. So I am removing this change from here.

@dims
Copy link
Member

dims commented Nov 10, 2016

@quinton-hoole @madhusudancs : looks like this needs a rebase at least. will this make it to v1.5? if not, can you please switch milestone?

This was referenced Nov 10, 2016
Closed
Closed
@madhusudancs
Copy link
Contributor Author

madhusudancs commented Nov 10, 2016 •
edited
Loading

@dims this is a bug fix for a feature introduced in v1.5, so this has to be in 1.5. I am aware that this needs a rebase and I need to address a review comment. Please give us some time. We are pretty occupied with KubeCon+Dev summit this week.

@dims
Copy link
Member

dims commented Nov 10, 2016

@madhusudancs Ack. This was just a first pass at figuring out what can/should remain on the milestone tag on behalf of the release czar. So thanks for your input. i agree with your reasoning.

madhusudancs
madhusudancs commented Nov 14, 2016
View reviewed changes
Copy link
Contributor Author

@madhusudancs madhusudancs left a comment

Choose a reason for hiding this comment

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

@quinton-hoole PTAL.

federation/pkg/kubefed/init/init.go
"--kubeconfig=/etc/federation/controller-manager/kubeconfig",
"--dns-provider=gce",
"--dns-provider=google-clouddns",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quinton-hoole @irfanurrehman fixed this in PR #36492. So I am removing this change from here.

@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2016
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 15, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 73f1127a2fd36fc1dd17885db5678a22b5496ab7. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@madhusudancs madhusudancs changed the title [Federation][init-10d] Use the right service and dns provider names in controller manager. [Federation][init-10d] Use the right service names in controller manager. Nov 15, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2016
@madhusudancs
Copy link
Contributor Author

Fixed the unit test failure, adding LGTM back.

@madhusudancs madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2016
@madhusudancs
Copy link
Contributor Author

@saad-ali this is a bug fix. Should this go through the exception process?

@dims
Copy link
Member

dims commented Nov 15, 2016

@madhusudancs milestone is set, lgtm is applied. so this will go through as you have already mentioned this was a "bug fix". So we are good i think.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@saad-ali
Copy link
Member

@saad-ali this is a bug fix. Should this go through the exception process?

Nope, bug fix ok for post-code freeze merge.

@madhusudancs
Copy link
Contributor Author

@dims @saad-ali thanks!

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d51e27f into kubernetes:master Nov 15, 2016
@madhusudancs madhusudancs deleted the federation-kubefed-init-10d branch November 16, 2016 02:07
@madhusudancs madhusudancs mentioned this pull request Nov 17, 2016
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees
No one assigned
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@madhusudancs @k8s-ci-robot @dims @k8s-github-robot @saad-ali @googlebot