-
Notifications
You must be signed in to change notification settings - Fork 36
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
Allow users to define vm naming logic #369
Conversation
d4dfafe
to
d70600d
Compare
@arunmk I added an immutability check to the webhook. This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I have only one ask of a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the error check, which seems like which was added by mistake, the rest of the PR looks good to me.
controllers/vcdmachine_controller.go
Outdated
@@ -441,7 +449,10 @@ func (r *VCDMachineReconciler) reconcileNormal(ctx context.Context, cluster *clu | |||
cloudInitInput.HTTPProxy = vcdCluster.Spec.ProxyConfigSpec.HTTPProxy | |||
cloudInitInput.HTTPSProxy = vcdCluster.Spec.ProxyConfigSpec.HTTPSProxy | |||
cloudInitInput.NoProxy = vcdCluster.Spec.ProxyConfigSpec.NoProxy | |||
cloudInitInput.MachineName = machine.Name | |||
cloudInitInput.MachineName = vmName | |||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not sure what error is being checked here. Can you please make sure this is not added by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erkanerol could you please handle this? we should be able to merge immediately after it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is leftover after a refactoring. Just deleted the check. Thanks for catching this.
326e6ba
to
20b05b8
Compare
Now, VMs are named as machine.Name. In some cases, we need to name VMs with a custom logic. This PR adds a new field into VCDMachine CR so that users can define templates to name VMs by using Go templates and Sprig functions. ".machine" and ".vcdMachine" will refer to Machine and VCDMachine CRs in the templates.
34db163
to
7603f5a
Compare
@arunmk @Anirudh9794 I rebased the PR. FYI. |
|
||
// VmNamingTemplate is go template to generate VM names based on Machine and VCDMachine CRs. | ||
// Functions of Sprig library are supported. See https://github.com/Masterminds/sprig | ||
// Immutable field. machine.Name is used as VM name when this field is empty. | ||
// +optional | ||
VmNamingTemplate string `json:"vmNamingTemplate"` | ||
VmNamingTemplate string `json:"vmNamingTemplate,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users should be able to create VCDMachine CRs without providing empty values for these fields. Missing omitempty tags lead to errors in CAPI controllers while creating VCDMachine by using VCDMachineTemplate.
I'll merge this since it has 2 approvals. |
Description
Now, VMs are named as
machine.Name
. In some cases, we need to name VMs with a custom logic.This PR adds a new field into VCDMachine CR so that users can define templates to name VMs by using Go templates and Sprig functions.
.machine
and.vcdMachine
will refer to Machine and VCDMachine CRs in the templates.Example
Important Points
This field must be immutable. Otherwise, users can change it and the controller will not be able to know existing VMs.
Immutability is enforced by the new validating webhook for VCDMachine.
Checklist
API Changes
Are there API changes?
If yes, please fill in the below
./examples/capi-quickstart.yaml
?./infrastructure-vcd/v1.0.0/
?This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)