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.

Sign up for GitHub

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

Update generate cluster config command for Tinkerbell #8226

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Update generate cluster config command for Tinkerbell #8226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41f0aab
Update generate cluster config command for Tinkerbell
May 29, 2024
65a6d63
Make os image url field on machine config non-optional
May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/api/v1alpha1/tinkerbelldatacenterconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
ObjectMeta: ObjectMeta{
Name: clusterName,
},
Spec: TinkerbellDatacenterConfigSpec{},
Spec: TinkerbellDatacenterConfigSpec{
TinkerbellIP: "",
},

Check warning on line 27 in pkg/api/v1alpha1/tinkerbelldatacenterconfig.go

View check run for this annotation

Codecov / codecov/patch

pkg/api/v1alpha1/tinkerbelldatacenterconfig.go#L25-L27 

Added lines #L25 - L27 were not covered by tests
}
}

Expand Down
1 change: 1 addition & 0 deletions pkg/api/v1alpha1/tinkerbellmachineconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
Spec: TinkerbellMachineConfigSpec{
HardwareSelector: HardwareSelector{},
OSFamily: Ubuntu,
OSImageURL: "",

Check warning on line 30 in pkg/api/v1alpha1/tinkerbellmachineconfig.go

View check run for this annotation

Codecov / codecov/patch

pkg/api/v1alpha1/tinkerbellmachineconfig.go#L30 

Added line #L30 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Users: []UserConfiguration{
{
Name: "ec2-user",
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/v1alpha1/tinkerbellmachineconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Member

Choose a reason for hiding this comment

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

Some versions of bottlerocket are still supported so we shouldn't do this until we stop supporting them fully

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

OSImageURL string `json:"osImageURL"`
Users []UserConfiguration `json:"users,omitempty"`
HostOSConfiguration *HostOSConfiguration `json:"hostOSConfiguration,omitempty"`
}
Expand Down
12 changes: 12 additions & 0 deletions pkg/providers/tinkerbell/assert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,18 @@ func TestAssertMachineConfigK8sVersionBRWorker_Error(t *testing.T) {
g.Expect(err).ToNot(gomega.Succeed())
}

func TestAssertMachineConfigK8sVersionBRModularWorker_Error(t *testing.T) {
g := gomega.NewWithT(t)
builder := NewDefaultValidClusterSpecBuilder()
clusterSpec := builder.Build()
kube129 := eksav1alpha1.Kube129
clusterSpec.Spec.Cluster.Spec.ExternalEtcdConfiguration = nil
clusterSpec.Spec.Cluster.Spec.WorkerNodeGroupConfigurations[0].KubernetesVersion = &kube129
clusterSpec.MachineConfigs[builder.WorkerNodeGroupMachineName].Spec.OSFamily = "bottlerocket"
err := tinkerbell.AssertOsFamilyValid(clusterSpec)
g.Expect(err).ToNot(gomega.Succeed())
}

func TestAssertMachineConfigK8sVersionBR_Success(t *testing.T) {
g := gomega.NewWithT(t)
builder := NewDefaultValidClusterSpecBuilder()
Expand Down
Loading