[WIP] Convert machineset templates to actual objects #567
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rajatchopra The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
…bjects Import the cluster-api v1alpha1 object types to be used by master/worker instances Import the cluster-api-aws-provider package for use by aws instance generation Provide the function to be used for building machine set instance list, instead of using templates
rajatchopra
force-pushed
the
clusterapi
branch
from
f77100e to
6f53ddc
Compare
|
@rajatchopra: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
|
/cc @abhinavdahiya |
| ms.Spec.Template = template | ||
|
|
||
| // add the machineset to items | ||
| items = append(items, runtime.RawExtension{Object: &ms}) |
There was a problem hiding this comment.
Why the local items vs.:
list.Items = append(list.Items, runtime.RawExtension{Object: &ms})?
There was a problem hiding this comment.
Just logically constructing the all the structures leaf-up along. No particular reason here.
There was a problem hiding this comment.
Just logically constructing the all the structures leaf-up along.
I'm fine with that approach if you want to shift the root type declaration down to the end of the function. But I'd rather not have both a beginning-of-the-function root declaration and leaf local variables.
| template.Spec.ProviderConfig = clusterapi.ProviderConfig{Value: &runtime.RawExtension{Object: &provider}} | ||
| template.Spec.Versions = clusterapi.MachineVersionInfo{ | ||
| Kubelet: "", | ||
| ControlPlane: "", |
There was a problem hiding this comment.
Does setting these to their zero value do anything? Do we need to populate Spec.Versions at all?
There was a problem hiding this comment.
That is how it was in the template. I am guessing we do want to write the correct values at some point. Next to no harm for now.
There was a problem hiding this comment.
That is how it was in the template.
Presumably because the upstream type is missing some omitempty.
I am guessing we do want to write the correct values at some point. Next to no harm for now.
I'd rather address that with FIXME comments in the code and/or GitHub issues to track the planned future work. Leaving empty strings here doesn't seem like an obvious way to track future work.
| } | ||
|
|
||
| // WorkerMachineSetsTmpl is template for worker machinesets. | ||
| var WorkerMachineSetsTmpl = template.Must(template.New("aws-worker-machinesets").Parse(` |
There was a problem hiding this comment.
Yes, forthcoming commit, when I tie this all up with other templates.
| "text/template" | ||
|
|
||
| awsprovider "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1alpha1" | ||
| clusterapi "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1" |
There was a problem hiding this comment.
Looks like an inconsistent indent issue here.
|
/close |
|
@abhinavdahiya: Closing this PR. DetailsIn response to this:
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. |
4 participants
The templates used to build machineset assets are being replaced with actual objects. The benefit will be across other projects as well as it is easier to work with objects rather than template string expansion.
TODO: