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

Detect the project in which the federation of clusters are being created and point the federation docker registry to that project. #26951

Merged
merged 3 commits into from
Jun 13, 2016
Merged

Detect the project in which the federation of clusters are being created and point the federation docker registry to that project. #26951

merged 3 commits into from
Jun 13, 2016

Conversation

madhusudancs
Copy link
Contributor

@madhusudancs madhusudancs commented Jun 7, 2016 •
edited
Loading

Only the last commit here needs review.

Depends on #26950.

cc @colhom @kubernetes/sig-cluster-federation

Analytics

@madhusudancs madhusudancs added area/cluster-federation release-note-none Denotes a PR that doesn't merit a release note. labels Jun 7, 2016
@madhusudancs madhusudancs added this to the v1.3 milestone Jun 7, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2016
@madhusudancs madhusudancs mentioned this pull request Jun 7, 2016
Merged
@colhom
Copy link

colhom commented Jun 7, 2016

@madhusudancs great idea, lgtm.

@nikhiljindal
Copy link
Contributor

Yes, thanks!
LGTM

@madhusudancs madhusudancs force-pushed the fed-detect-project-registry-base branch from 25056c0 to 0a79f5c Compare June 7, 2016 19:13
@madhusudancs madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2016
@madhusudancs
Copy link
Contributor Author

Rebased the PR to only contain the relevant commit. Added LGTM label.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2016
@colhom colhom mentioned this pull request Jun 8, 2016
Merged
nikhiljindal
nikhiljindal reviewed Jun 8, 2016
View reviewed changes
federation/cluster/common.sh

# Populates $PROJECT
detect-project
if [[ ${PROJECT} == *':'* ]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt this also check that FEDERATION_PUSH_REPO_BASE is empty? Otherwise, I will always get this error if my project has ":" in it (which I think is true for @quinton-hoole's project)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also optional: we dont need to run this whole code (including calling detect-project) if FEDERATION_PUSH_REPO_BASE is set already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. Fixed.

@nikhiljindal
Copy link
Contributor

Removing LGTM as per the comment above.
If submitted as is, then I think this will break @quinton-hoole

@nikhiljindal nikhiljindal removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2016
k8s-github-robot pushed a commit that referenced this pull request Jun 10, 2016
Merge pull request #26952 from madhusudancs/fed-apiserver-admissionco…
9ae6a00 
…ntrol-todo

Automatic merge from submit-queue

Add a TODO to enable admission control in federation apiserver when the support is added.

Only the last commit here needs review.

Depends on #26951.

cc @kubernetes/sig-cluster-federation

[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()
k8s-github-robot pushed a commit that referenced this pull request Jun 10, 2016
Merge pull request #27072 from colhom/federation-e2e-docs
6a388d4 
Automatic merge from submit-queue

Updating e2e docs with instructions on running federation tests

Last two commits are for review. Depends on #26951

\cc @madhusudancs @quinton-hoole @nikhiljindal 
 
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()
@madhusudancs madhusudancs force-pushed the fed-detect-project-registry-base branch from 0a79f5c to 498168b Compare June 10, 2016 07:22
@madhusudancs
Copy link
Contributor Author

@nikhiljindal Fixed. PTAL.

nikhiljindal
nikhiljindal reviewed Jun 10, 2016
View reviewed changes
federation/cluster/common.sh

# If $FEDERATION_PUSH_REPO_BASE isn't set set the GCR registry name based on the
# detected project name.
if [[ -z "${FEDERATION_PUSH_REPO_BASE}" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, should we also check that KUBERNETES_PROVIDER = gce or gke?
I assume this wont work on vagrant and aws?

@nikhiljindal
Copy link
Contributor

We should also update the documentation that colin added to say that the repo env var is optional on gce and gke.
Fine to do that in a separate PR.

@nikhiljindal
Copy link
Contributor

Feel free to add the LGTM label once you have added the KUBERNETES_PROVIDER check

@madhusudancs madhusudancs force-pushed the fed-detect-project-registry-base branch from e439ec2 to c7a4401 Compare June 13, 2016 09:09
@madhusudancs
Copy link
Contributor Author

Addressed the comments. Adding LGTM.

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

fejta commented Jun 13, 2016

@k8s-bot e2e test this issue: #27266

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Jun 13, 2016

GCE e2e build/test passed for commit c7a4401.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 10255f8 into kubernetes:master Jun 13, 2016
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
Merge pull request kubernetes#27072 from colhom/federation-e2e-docs
c520a56 
Automatic merge from submit-queue

Updating e2e docs with instructions on running federation tests

Last two commits are for review. Depends on kubernetes#26951

\cc @madhusudancs @quinton-hoole @nikhiljindal 
 
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
Merge pull request kubernetes#26951 from madhusudancs/fed-detect-proj…
0c80872 
…ect-registry-base

Automatic merge from submit-queue

Detect the project in which the federation of clusters are being created and point the federation docker registry to that project.

Only the last commit here needs review.

Depends on kubernetes#26950.

cc @colhom @kubernetes/sig-cluster-federation

[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikhiljindal nikhiljindal

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
7 participants
@madhusudancs @colhom @nikhiljindal @fejta @k8s-github-robot @k8s-bot @googlebot