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.

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

[Federation] Remove unnecessary functions from develop.sh as part of … #38902

Merged

Conversation

madhusudancs
Copy link
Contributor

This is part of a big refactor to deprecate a short-lived deploy.sh
mechanism that nobody really could use due to bugs.

`federation/deploy/deploy.sh` was an interim solution introduced in Kubernetes v1.4 to simplify the federation control plane deployment experience. Now that we have `kubefed`, we are deprecating `deploy.sh` scripts.

cc @kubernetes/sig-federation-misc @irfanurrehman

…deploy.sh deprecation.

This is part of a big refactor to deprecate a short-lived deploy.sh
mechanism that nobody really could use due to bugs.
@madhusudancs madhusudancs added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/federation labels Dec 16, 2016
@madhusudancs madhusudancs added this to the v1.6 milestone Dec 16, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 16, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 16, 2016
@@ -123,7 +115,9 @@ function get_version() {
kube_version="${KUBE_VERSION}"
else
# Read the version back from the versions file if no version is given.
kube_version="$(jq -r '.KUBE_VERSION' ${VERSIONS_FILE})"
kube_version="$(cat ${VERSIONS_FILE} | python -c '\
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this is the best way to do this.
Feel free to cc someone else who would know more about 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.

cc @ixdy.

This was a page taken straight out of @ixdy's book - kubernetes/release#207


# Update config after build and push, but before turning up the clusters
# to ensure the config has the right image version tags.
gen_or_update_config "${kube_version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry why is this not required now?
It is deprecated but we dont want to intentionally break it :)

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 function was only required to generate the config for deploying clusters, not the federation control plane. I am considering completely getting rid of the cluster deployment part.

@nikhiljindal
Copy link
Contributor

To clarify this is a cleanup PR. Does not fix a specific bug/issue, right?

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.

@nikhiljindal this is a bug fix. This fixes the test build failures we are having right now. This is a follow up to - PR #38888


# Update config after build and push, but before turning up the clusters
# to ensure the config has the right image version tags.
gen_or_update_config "${kube_version}"
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 function was only required to generate the config for deploying clusters, not the federation control plane. I am considering completely getting rid of the cluster deployment part.

@nikhiljindal
Copy link
Contributor

Thanks for the clarifications
LGTM

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 37468, 36546, 38713, 38902, 38614)

@k8s-github-robot k8s-github-robot merged commit 9929fd3 into kubernetes:master Dec 17, 2016
@madhusudancs madhusudancs deleted the fed-newtest-build-no-jq branch February 25, 2017 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants