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

Add templateOverride field to override the default workflowTemplate #130

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

abhinavmpandey08
Copy link
Contributor

Description

This PR adds a new field templateOverride to TinkerbellMachine CRD to override the default CAPT workflow template https://github.com/tinkerbell/cluster-api-provider-tinkerbell/blob/main/internal/templates/templates.go#L56-L122

Why is this needed

Currently, the workflow template for Tinkerbell machines is hardcoded and there isn't a way to provide a custom template. If a user wants to provide a customized template with different actions, CAPT currently doesn't provide a way to do that.

How Has This Been Tested?

Tested the changes by passing a custom template using the templateOverride field and verified that the custom template works properly and provisions a CAPT cluster.
I tested the changes on bare-metal nodes and added some custom actions along with the default ones in the template.

How are existing users impacted? What migration steps/scripts do we need?

This change doesn't impact existing users. The templateOverride field is optional and if a user doesn't specify it, the default CAPT template is used instead.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Copy link
Contributor

@detiber detiber left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this PR and apologies for the long turnaround on review my github notifications are a bit of a mess right now 😬.

Outside of the comments I made on the PR I do have a few other questions, mostly about how you believe this feature would be used in practice.

  1. Would you expect it be common for users to want to override the template on only a subset of Machines, or would it also be likely for users to want to override the template for all Machines in a given Cluster?

  2. Do you see this as more of a full rip/replace of the created template, or do you think users would likely want to add additional actions at pre-defined extension points (for example additionalVolumes, preStreamImage, postStreamImage, preKexec, etc)?

api/v1beta1/tinkerbellmachine_types.go Outdated Show resolved Hide resolved
controllers/machine.go Outdated Show resolved Hide resolved
@abhinavmpandey08
Copy link
Contributor Author

Thank you for submitting this PR and apologies for the long turnaround on review my github notifications are a bit of a mess right now 😬.

Outside of the comments I made on the PR I do have a few other questions, mostly about how you believe this feature would be used in practice.

  1. Would you expect it be common for users to want to override the template on only a subset of Machines, or would it also be likely for users to want to override the template for all Machines in a given Cluster?
  2. Do you see this as more of a full rip/replace of the created template, or do you think users would likely want to add additional actions at pre-defined extension points (for example additionalVolumes, preStreamImage, postStreamImage, preKexec, etc)?

Hi @detiber, thanks for the review!

  1. That will depend on the use case. Users may want to have different template actions for control plane and worker nodes so having that flexibility would be nice. For example, users may want to mount custom certificates for private registry just in the worker nodes so their workloads can pull the images from it.
  2. My plan for this change was to give users the ability to replace this whole template entirely with their own custom templates. The default template is nice for say POC but power users may want to configure more things

additional actions at pre-defined extension points (for example additionalVolumes, preStreamImage, postStreamImage, preKexec, etc)

I was thinking of initially doing that but there are also some other things that users may want to change in the default template as well like use image2disk to stream the image instead of oci2disk.
So, with templateOverride, they can provide the entire template itself where they will be able to add custom actions
as well as customize the default actions.
Also, I have made it so that templateOverride is optional and if a user does not want to mess around with it, they can just leave it and CAPT would work the same way as it did before

Signed-off-by: Abhinav Pandey <abhinavmpandey08@gmail.com>
Signed-off-by: Abhinav Pandey <abhinavmpandey08@gmail.com>
@detiber
Copy link
Contributor

detiber commented Feb 1, 2022

@Mergifyio refresh

@detiber detiber added the ready-to-merge Signal to Mergify to merge the PR. label Feb 1, 2022
@mergify
Copy link
Contributor

mergify bot commented Feb 1, 2022

refresh

✅ Pull request refreshed

@detiber detiber merged commit b011d5e into tinkerbell:main Feb 1, 2022
mergify bot added a commit that referenced this pull request Aug 23, 2022
My team has been actively working on this project. My approver role will help expedite the PR review/approve process and help unblock any open issues or feature requests.

Org request issue: tinkerbell/org#24

Requirements:

* I have reviewed the [community membership guidelines](https://github.com/tinkerbell/proposals/blob/main/proposals/0024/GOVERNANCE.md)
* I have [enabled 2FA on my GitHub account](https://github.com/settings/security)
* I have subscribed to the [tinkerbell-contributors e-mail list](https://groups.google.com/g/tinkerbell-contributors)
* I am actively contributing to 1 or more Tinkerbell subprojects

PRs contributions on CAPT:
- #130
- #176

PRs reviewed on CAPT:
- #160
- #184
- #182

Sponsors:
@micahhausler @chrisdoherty4 @jacobweinstock
@displague displague added this to the v0.2 milestone Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants