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

Docker cluster template #308

Merged
merged 3 commits into from
Jan 19, 2022

Conversation

anusha94
Copy link
Contributor

@anusha94 anusha94 commented Jan 11, 2022

What this PR does / why we need it:
Creating separate cluster templates in the release artifacts - one for docker based byohosts and the other for vm based byohosts
e2e cluster templates are in a separate e2e directory - no release artifacts for this

Test release on my forked repo - https://github.com/anusha94/cluster-api-provider-bringyourownhost/releases/tag/v0.0.7
Diff:

kokoni at kangaroo in ~ ah
$ diff cluster-template.yaml new-artifacts/cluster-template.yaml
156c156
<   bundleLookupTag: v0.1.0_alpha.2
---
>   bundleLookupTag: ${BUNDLE_LOOKUP_TAG}

kokoni at kangaroo in ~ ah
$ diff cluster-template-docker.yaml new-artifacts/cluster-template-docker.yaml
18,20d17
<   labels:
<     cni: ${CLUSTER_NAME}-crs-0
<     crs: "true"
167c164
<   bundleLookupTag: v0.1.0_alpha.2
---
>   bundleLookupTag: ${BUNDLE_LOOKUP_TAG}

Fixes #277

Copy link
Contributor

@jamiemonserrate jamiemonserrate left a comment

Choose a reason for hiding this comment

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

Could kustomize help avoid doing everything in sed? We seem to use it elsewhere. Any reason we not using it here?

@anusha94
Copy link
Contributor Author

Could kustomize help avoid doing everything in sed?

This did cross my mind. But looks like kustomize edit can do only so many things

$ kustomize edit --help
Edits a kustomization file

Usage:
  kustomize edit [command]

Available Commands:
  add                       Adds an item to the kustomization file.
  alpha-list-builtin-plugin [Alpha] List the builtin plugins
  fix                       Fix the missing fields in kustomization file
  remove                    Removes items from the kustomization file.
  set                       Sets the value of different fields in kustomization file.

remove is to remove items from kustomization file and not from the files referenced there.

Even if we consider setting fields to empty instead of removing them, kustomize edit set is limited to a few fields.

$ kustomize edit set --help
Sets the value of different fields in kustomization file.

Usage:
  kustomize edit set [command]

Available Commands:
  annotation  Sets one or more commonAnnotations in kustomization.yaml
  image       Sets images and their new names, new tags or digests in the kustomization file
  label       Sets one or more commonLabels in kustomization.yaml
  nameprefix  Sets the value of the namePrefix field in the kustomization file.
  namespace   Sets the value of the namespace field in the kustomization file
  namesuffix  Sets the value of the nameSuffix field in the kustomization file.
  replicas    Sets replicas count for resources in the kustomization file

But I do hear you. Doing so many sed & perl commands on a release artifact may not always be reliable. How about we maintain 2 sets of cluster templates in the repo itself?
The current one is for docker here. We can have another folder containing vm related templates. Only downside being if there's any change in the API or defaults, we need to update in 2 places. Hoping these won't be as frequent, this could be a good trade-off I guess.

Thoughts? @jamiemonserrate @dharmjit

@dharmjit
Copy link
Contributor

dharmjit commented Jan 12, 2022

Hoping these won't be as frequent, this could be a good trade-off I guess

Having explicit templates for VM/docker would make it easier to understand the generation of different flavors of cluster-template release artifacts. I agree it will add some more maintenance but should be okay I guess.

@jamiemonserrate
Copy link
Contributor

Agree with where this thread is heading.

I can see that the AWS provider is doing something similar - that is, they have a kustomize_sources folder, just like we are planning to have.

So that reassures me that we are heading in the right direction 😄

@anusha94 anusha94 force-pushed the docker-cluster-template branch 2 times, most recently from 1a32fdb to cd6fee4 Compare January 13, 2022 12:30
Makefile Outdated Show resolved Hide resolved
dharmjit
dharmjit previously approved these changes Jan 14, 2022
Copy link
Contributor

@dharmjit dharmjit left a comment

Choose a reason for hiding this comment

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

lgtm!

- create new set of templates for vm based hosts
- update GH action to include multiple cluster templates in release
  artifacts
- update make tasks to generate multiple cluster templates
@anusha94 anusha94 requested a review from huchen2021 January 18, 2022 04:14
Makefile Show resolved Hide resolved
@jamiemonserrate jamiemonserrate merged commit 7ffa268 into vmware-tanzu:main Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish different cluster-template artifacts for Docker and everything else
4 participants