Addressing comments regarding documentation, missing docstring, unnecessary conversions

Author: dulek

api/v1alpha7/conversion_test.go

@@ -64,7 +64,6 @@ func TestFuzzyConversion(t *testing.T) {
 			func(v1alpha8OpenStackMachine *infrav1.OpenStackMachine, c fuzz.Continue) {
 				c.FuzzNoCustom(v1alpha8OpenStackMachine)
 
-				// None of the following fields have ever been set in v1alpha7
 				v1alpha8OpenStackMachine.Status.ReferencedResources = infrav1.ReferencedMachineResources{}
 			},
 

api/v1alpha8/types.go

@@ -363,8 +363,10 @@ type APIServerLoadBalancer struct {
 	Provider string `json:"provider,omitempty"`
 }
 
-// ReferencedMachineResources contains resolved references to resources required by a machine.
+// ReferencedMachineResources contains resolved references to resources required by the machine.
 type ReferencedMachineResources struct {
+	// ServerGroupID is the ID of the server group the machine should be added to and is calculated based on ServerGroupFilter.
+	// +optional
 	ServerGroupID string `json:"serverGroupID,omitempty"`
 }
 

docs/book/src/development/development.md

@@ -341,3 +341,13 @@ $ sudo ln -s /run/user/$(id -u)/podman/podman.sock /var/run/docker.sock
 [hack-ci-create-devstack]: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/hack/ci/create_devstack.sh
 [hack-ci-gce-project]: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/hack/ci/gce-project.sh
 [hack-ci-openstack]: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/hack/ci/openstack.sh
+
+## API concepts
+
+This sections goal is to gather various insights into the API design that can serve as a reference to explain various choices made without need to analyze discussions in individual PRs.
+
+### `referencedResources`
+
+Starting from v1alpha8 both `OpenStackMachineStatus` and `BastionsStatus` feature a field named `referencedResources` which aims to include fields that list individual IDs of the resources associated with the machine or bastion. These IDs are calculated on machine or bastion creation and are not intended to be changed during the object lifecycle.
+
+Having all the IDs of related resources saved in the statuses allows CAPO to make easy decisions about deleting the related resources when deleting the VM corresponding to the machine or bastion.

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md

@@ -23,9 +23,11 @@ This only documents backwards incompatible changes. Fields that were added to v1
 
 ### `OpenStackMachine`
 
-#### Change to `serverGroupID`
+#### ⚠️ Change to `serverGroupID`
 
-The `spec`'s field `serverGroupID` has been renamed to `serverGroup` and isn't a string anymore but a reference to a `ServerGroupFilter` object.
+The field `serverGroupID` has been renamed to `serverGroup` and is now a `ServerGroupFilter` object rather than a string ID.
+
+The `ServerGroupFilter` object allows selection of a server group by name or by ID.
 
 ```yaml
 serverGroupID: "e60f19e7-cb37-49f9-a2ee-0a1281f6e03e"
@@ -38,7 +40,7 @@ serverGroup:
   id: "e60f19e7-cb37-49f9-a2ee-0a1281f6e03e"
 ```
 
-It is now possible to specify a `ServerGroupFilter` object to select the external network to use for the cluster. The `ServerGroupFilter` object allows to select the server group by name or by ID.
+To select a server group by name instead of ID:
 
 ```yaml
 serverGroup:

pkg/cloud/services/compute/referenced_resources.go

@@ -21,6 +21,9 @@ import (
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 )
 
+// ResolveReferencedMachineResources is responsible for populating ReferencedMachineResources with IDs of
+// the resources referenced in the OpenStackMachineSpec by querying the OpenStack APIs. It'll return error
+// if resources cannot be found or their filters are ambiguous.
 func ResolveReferencedMachineResources(scope scope.Scope, spec *infrav1.OpenStackMachineSpec, resources *infrav1.ReferencedMachineResources) error {
 	compute, err := NewService(scope)
 	if err != nil {

pkg/cloud/services/compute/referenced_resources_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 The Kubernetes Authors.
+Copyright 2024 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -62,7 +62,7 @@ func Test_ResolveReferencedMachineResources(t *testing.T) {
 			wantErr:           false,
 		},
 		{
-			testName:          "Server group ID not found",
+			testName:          "Server group by Name not found",
 			serverGroupFilter: &infrav1.ServerGroupFilter{Name: "test-server-group"},
 			expect: func(m *mock.MockComputeClientMockRecorder) {
 				m.ListServerGroups().Return(

pkg/cloud/services/compute/servergroup_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 The Kubernetes Authors.
+Copyright 2024 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.