Skip to content

Generate seperate multi-node and single-node yaml#476

Merged
k8s-ci-robot merged 3 commits intokubernetes-sigs:masterfrom
prankul88:seperate-generate-files
Oct 21, 2019
Merged

Generate seperate multi-node and single-node yaml#476
k8s-ci-robot merged 3 commits intokubernetes-sigs:masterfrom
prankul88:seperate-generate-files

Conversation

@prankul88
Copy link
Contributor

What this PR does / why we need it:

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 #473

Special notes for your reviewer:

  1. As suggested by @sbueringer , I created separate folders for single-node/multi-node cluster and controlplane.
  2. generate.yaml takes input "single-node" /"multi-node" and generates yaml accordingly

Release note:


@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 1, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @prankul88. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 1, 2019
@sbueringer
Copy link
Member

sbueringer commented Oct 6, 2019

@prankul88
Sorry for the late review. I also want to test the new generated files sometime in the next week. Don't have time for that right now.

Can you also do the following changes in controlplane.yaml

  • Remove these blocks:
    • multi
  # multi-node control-plane:
  # * Disable the floatingIP property
  # single-node control-plane:
  # * Enable the floatingIP property
  #floatingIP: <floating IP>
  • single
  # multi-node control-plane:
  # * Disable the floatingIP property
  # single-node control-plane:
  # * Enable the floatingIP property
  • There is some documentation around: clusterConfiguration.controlPlaneEndpoint. Let's move that inside the value of controlPlaneEndpoint (which would be <loadbalancer ip> (multi) and <floating ip of control plane node> (single) respectively

@sbueringer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 6, 2019
@prankul88
Copy link
Contributor Author

/retest

@sbueringer
Copy link
Member

@prankul88 test failure is fixed on my pr. See the changes in the Makefile: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/477/files

This was caused by a change in the kustomize release process.

@sbueringer
Copy link
Member

@prankul88 Very nice overall :). Only the folder rename is left and it would make sense to use the test fix from my PR so you don't have to wait until mine is merged.

@sbueringer sbueringer mentioned this pull request Oct 9, 2019
6 tasks
@prankul88
Copy link
Contributor Author

@sbueringer Great. Thanks for bringing out the job failure reason. I will update the changes soon.

@sbueringer
Copy link
Member

@prankul88 You can also rebase on master for the test fix

@prankul88 prankul88 force-pushed the seperate-generate-files branch from e601038 to 5c6ba84 Compare October 10, 2019 10:36
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2019
@prankul88
Copy link
Contributor Author

prankul88 commented Oct 11, 2019

@sbueringer Sorry did not see your network patch got merged. I guess l have to rebase and update the controlplane.yaml according to #477

@prankul88 prankul88 force-pushed the seperate-generate-files branch from 36a4a08 to fa62919 Compare October 11, 2019 07:17
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2019
@prankul88 prankul88 force-pushed the seperate-generate-files branch 2 times, most recently from 3e6d872 to e56c5f0 Compare October 11, 2019 07:29
@sbueringer
Copy link
Member

@sbueringer Sorry did not see your network patch got merged. I guess l have to rebase and update the controlplane.yaml according to #477

You don't have to change anything because of #477 . It's just another variant is supported if it's not allowed to create networks and subnets in OpenStack. I would like to keep the default the same as it is right now

@sbueringer
Copy link
Member

@prankul88 so only the paramter change is left, right? Could you also update the script documentation in examples/README.md?

@prankul88 prankul88 force-pushed the seperate-generate-files branch from e56c5f0 to 803aaf6 Compare October 17, 2019 04:09
@sbueringer
Copy link
Member

sbueringer commented Oct 20, 2019

@prankul88 It would be far easier to review if you just push individual commits :). By force pushing an individual commit it's hard to figure out what actually changed. Prow will squash on merge anyway.

@prankul88
Copy link
Contributor Author

@sbueringer sorry, I would push them individually from now on. Would like to clarify the latest commit I have pushed.

  1. Have updated the controlpane.yaml and machinedeployments.yaml ( according to Pr use existing networks #477 )
  2. Updated generate.sh which makes <output dir> as mandatory input and "single-node" as the default parameter.
  3. Updated README file accordingly

Kindly take a look.

@sbueringer
Copy link
Member

@prankul88 Of course. Just wanted to tell you that it's easier that way :)

@sbueringer
Copy link
Member

@prankul88 Looks good. Apparently they changed something in the upstream ClusterAPI repos. Can you replace l.223-229 in generate.sh with:

# Generate Cluster API provider components file.
CAPI_BRANCH=${CAPI_BRANCH:-"stable"}
if [[ ${CAPI_BRANCH} == "stable" ]]; then
  curl -L https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.2.4/cluster-api-components.yaml > "${COMPONENTS_CLUSTER_API_GENERATED_FILE}"
  echo "Downloaded ${COMPONENTS_CLUSTER_API_GENERATED_FILE} from cluster-api stable branch - v0.2.4"
else
  kustomize build "github.com/kubernetes-sigs/cluster-api/config/default/?ref=${CAPI_BRANCH}" > "${COMPONENTS_CLUSTER_API_GENERATED_FILE}"
  echo "Generated ${COMPONENTS_CLUSTER_API_GENERATED_FILE} from cluster-api - ${CAPI_BRANCH}"
fi

# Generate Kubeadm Bootstrap Provider components file.
CABPK_BRANCH=${CABPK_BRANCH:-"stable"}
if [[ ${CABPK_BRANCH} == "stable" ]]; then
  curl -L https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/releases/download/v0.1.2/bootstrap-components.yaml > "${COMPONENTS_KUBEADM_GENERATED_FILE}"
  echo "Downloaded ${COMPONENTS_KUBEADM_GENERATED_FILE} from cluster-api-bootstrap-provider-kubeadm stable branch - v0.1.2"
else
  kustomize build "github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/config/default/?ref=${CABPK_BRANCH}" > "${COMPONENTS_KUBEADM_GENERATED_FILE}"
  echo "Generated ${COMPONENTS_KUBEADM_GENERATED_FILE} from cluster-api-bootstrap-provider-kubeadm - ${CABPK_BRANCH}"
fi

Everything else lgtm :)

@prankul88
Copy link
Contributor Author

@sbueringer Have updated !! PTAL

@sbueringer
Copy link
Member

Perfect. Thx very much :-)

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: prankul88, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Oct 21, 2019
@k8s-ci-robot k8s-ci-robot merged commit 4469d4e into kubernetes-sigs:master Oct 21, 2019
pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024
* Generate seperate multi-node and single-node yaml

Updated documentation

* Update controlplane and machinedeployment.yaml

* Update generate.sh with upstream Cluster-API repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generate separate multi-node and single-node control plane examples

4 participants