Update generate cluster config command for Tinkerbell #8226
Conversation
Signed-off-by: Rahul Ganesh <rahulgab@amazon.com>
eks-distro-bot
added
the
size/S
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8226 +/- ##
==========================================
- Coverage 73.42% 73.41% -0.01%
==========================================
Files 577 577
Lines 35944 35947 +3
==========================================
Hits 26391 26391
- Misses 7884 7887 +3
Partials 1669 1669 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rahul Ganesh <rahulgab@amazon.com>
| @@ -18,7 +18,7 @@ type TinkerbellMachineConfigSpec struct { | |||
| // OSImageURL is a URL to the OS image used during provisioning. It must include | |||
| // the Kubernetes version(s). For example, a URL used for Kubernetes 1.27 could | |||
| // be http://localhost:8080/ubuntu-2204-1.27.tgz | |||
| OSImageURL string `json:"osImageURL,omitempty"` | |||
There was a problem hiding this comment.
Some versions of bottlerocket are still supported so we shouldn't do this until we stop supporting them fully
There was a problem hiding this comment.
Yes, they are still supported. But we already made that API change on data center config in this PR . So this PR is merely trying to make it consistent. And don't think again toggling them back to optional makes sense here given we are eventually deprecating.
| @@ -27,6 +27,7 @@ func NewTinkerbellMachineConfigGenerate(name string, opts ...TinkerbellMachineCo | |||
| Spec: TinkerbellMachineConfigSpec{ | |||
| HardwareSelector: HardwareSelector{}, | |||
| OSFamily: Ubuntu, | |||
| OSImageURL: "", | |||
There was a problem hiding this comment.
any reason why we are exposing this in the machine config instead of datacenter config? I would think most users would use OSImageURL there generally
There was a problem hiding this comment.
So basically when we defaulted to ubuntu we missed adding OSImageURL field. For modular upgrades machine configs is expected in OSImageURL field. Also, this is more akin to how we set templated at machine config level in other providers. Once we introduced OSImageURL on machine config level the plan was to give preference to this field and eventually deprecate OSImageURL field at Datacenter config. But we haven't reached there yet!
|
/hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavmpandey08 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override codecov/patch |
|
@rahulbabu95: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In 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. |
Issue #, if available:
Update generate command for Tinkerbell to populate Tinkerbell IP in the Datacenter config and OSImage URL in Machine config object as we have defaulted the OS from BR to Ubuntu. This change also adds a check when a modular K8s version is specified with a value above 1.28 and the OS family is set to BR.
Description of changes:
Testing (if applicable):
Documentation added/planned (if applicable):
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.