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

Updating federation up scripts to work in non e2e setup #27260

Merged
merged 3 commits into from
Jun 14, 2016
Merged

Updating federation up scripts to work in non e2e setup #27260

merged 3 commits into from
Jun 14, 2016

Conversation

nikhiljindal
Copy link
Contributor

Ref: kubernetes/website#656

Updating the federation up scripts so that they work as per steps in kubernetes/website#656.

Changes are:

  • Updating the default namespace to be "federation" instead of "federation-e2e"
  • Updated the kubeconfig context to be named "federation-cluster" instead of "federated-context"
  • Fixing federation-up so that FEDERATION_IMAGE_TAG is set even when federation-up is run without running e2e.go --up. e2e-up.sh sets it here:

    kubernetes/hack/e2e-internal/e2e-up.sh

    Line 44 in 6a388d4

    export FEDERATION_IMAGE_TAG="$(cat "${KUBE_ROOT}/federation/manifests/federated-image.tag")"
    .
  • Adding a "missingkey=zero" option to template parser. Without this, the parser adds "<no value>" at the place of an env var that is not set. With this change, it instead replaces it with the corresponding zero value (for ex "" for strings). This is required for the FEDERATION_DNS_PROVIDER_CONFIG env var.

cc @kubernetes/sig-cluster-federation @colhom @mml

@nikhiljindal nikhiljindal added this to the v1.3 milestone Jun 13, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jun 13, 2016
@nikhiljindal nikhiljindal added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 13, 2016
@dchen1107 dchen1107 assigned ghost Jun 13, 2016
@ghost
Copy link

ghost commented Jun 13, 2016

LGTM! Thanks @nikhiljindal

@colhom
Copy link

colhom commented Jun 13, 2016

@nikhiljindal you're missing the test/ side of things which consumes these constants.

  • test/e2e/{framework/test_context,federation-apiserver}.go contain hard-coded references to old federated-cluster kubecontext name
  • test/e2e/framework/{util.framework}.go have hard-coded references to old federation-e2e namespace name.

@ghost ghost added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 13, 2016
@colhom
Copy link

colhom commented Jun 13, 2016

@quinton-hoole please remove lgtm label. This PR is not complete and will break the tests.

@ghost
Copy link

ghost commented Jun 13, 2016

Good point @colhom - removed LGTM until fixed.

@nikhiljindal
Copy link
Contributor Author

Thanks @quinton-hoole and @colhom
Updated the code to fix those references

@nikhiljindal nikhiljindal mentioned this pull request Jun 13, 2016
Closed
20 tasks
colhom
colhom reviewed Jun 13, 2016
View reviewed changes
federation/cluster/federation-up.sh
@@ -22,4 +22,8 @@ KUBE_ROOT=$(readlink -m $(dirname "${BASH_SOURCE}")/../../)

. ${KUBE_ROOT}/federation/cluster/common.sh

if [[ -f "${KUBE_ROOT}/federation/manifests/federated-image.tag" ]]; then
Copy link

@colhom colhom Jun 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This has been something I've been meaning to do. We actually can be very specific here:

tagfile="${KUBE_ROOT}/federation/manifests/federated-image.tag"
if [[ ! -f "$tagfile" && "${FEDERATION:-}" == "true ]];then
    echo "FATAL: FEDERATION=true, but tagfile ${tagfile} does not exist"
    exit 1
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
We do not check if FEDERATION=true in federation-up.sh

@colhom
Copy link

colhom commented Jun 13, 2016

@nikhiljindal there is one more old namespace reference in ./hack/e2e-internal/e2e-status.sh

@nikhiljindal
Copy link
Contributor Author

Thanks @colhom
Fixed and pushed a new version.
PTAL.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 13, 2016
@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2016
@nikhiljindal nikhiljindal added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 13, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2016
@nikhiljindal
Copy link
Contributor Author

Found a missed reference while testing. Fixed it now.
Also added a test to verify that cluster is marked ready by the controller.

Verified that the test passes!

@nikhiljindal
Copy link
Contributor Author

Adding back LGTM

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

Fixed verify-govet

@nikhiljindal nikhiljindal added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 14, 2016
@nikhiljindal
Copy link
Contributor Author

Merge both is not showing this PR in the merge queue.

Trying to run the e2e test again, to see if that triggers the merge bot to look at this again.

@k8s-bot e2e test this issue: #IGNORE

@k8s-bot
Copy link

k8s-bot commented Jun 14, 2016

GCE e2e build/test passed for commit 5a20112.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7e2b523 into kubernetes:master Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

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