Repository: metal3-io/baremetal-operator
Pull request #1310 (2023-07-11 07:37:52)

:bug: Remove duplicate declaration of annotation constants outside metal3api
Remove duplicate declaration of annotation constants in Controller

<!-- please add a icon to the title of this PR and delete this line and similar ones -->
<!-- the icon will be either ⚠️ (:warning:, major or breaking changes), ✨ (:sparkles:, feature additions), 🐛 (:bug:, patch and bugfixes), 📖 (:book:, documentation or proposals), or 🌱 (:seedling:, minor or other) -->

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #



Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index 1e96d64c..ec2a68e3 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -48,18 +48,18 @@ const (
 	// from the status annotation.
 	StatusAnnotation = "baremetalhost.metal3.io/status"
 
-	// RebootAnnotation is the annotation which tells the host which mode to use
+	// RebootAnnotationPrefix is the annotation which tells the host which mode to use
 	// when rebooting - hard/soft
 	RebootAnnotationPrefix = "reboot.metal3.io"
 
-	// InspectAnnotation is used to specify if automatic introspection carried out
+	// InspectAnnotationPrefix is used to specify if automatic introspection carried out
 	// during registration of BMH is enabled or disabled
-	InspectAnnotation = "inspect.metal3.io"
+	InspectAnnotationPrefix = "inspect.metal3.io"
 
 	// HardwareDetailsAnnotation provides the hardware details for the host
 	// in case its not already part of the host status and when introspection
 	// is disabed
-	HardwareDetailsAnnotation = "inspect.metal3.io/hardwaredetails"
+	HardwareDetailsAnnotation = InspectAnnotationPrefix + "/hardwaredetails"
 )
 
 // RootDeviceHints holds the hints for specifying the storage location
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index b45dd2c2..6b453bae 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -192,10 +192,10 @@ func validateAnnotations(host *BareMetalHost) []error {
 			err = validateStatusAnnotation(value)
 		case strings.HasPrefix(annotation, RebootAnnotationPrefix+"/") || annotation == RebootAnnotationPrefix:
 			err = validateRebootAnnotation(value)
-		case annotation == InspectAnnotation:
+		case annotation == InspectAnnotationPrefix:
 			err = validateInspectAnnotation(value)
 		case annotation == HardwareDetailsAnnotation:
-			inspect := host.Annotations[InspectAnnotation]
+			inspect := host.Annotations[InspectAnnotationPrefix]
 			err = validateHwdDetailsAnnotation(value, inspect)
 		default:
 			err = nil
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 10b127dd..991f7345 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -57,9 +57,6 @@ const (
 	preprovImageRetryDelay        = time.Minute * 5
 	provisionerNotReadyRetryDelay = time.Second * 30
 	subResourceNotReadyRetryDelay = time.Second * 60
-	rebootAnnotationPrefix        = "reboot.metal3.io"
-	inspectAnnotationPrefix       = "inspect.metal3.io"
-	hardwareDetailsAnnotation     = inspectAnnotationPrefix + "/hardwaredetails"
 	clarifySoftPoweroffFailure    = "Continuing with hard poweroff after soft poweroff fails. More details: "
 	hardwareDataFinalizer         = metal3api.BareMetalHostFinalizer + "/hardwareData"
 )
@@ -284,8 +281,8 @@ func (r *BareMetalHostReconciler) updateHardwareDetails(request ctrl.Request, ho
 	// We either just processed the annotation, or the status is already set
 	// so we remove it
 	annotations := host.GetAnnotations()
-	if _, present := annotations[hardwareDetailsAnnotation]; present {
-		delete(host.Annotations, hardwareDetailsAnnotation)
+	if _, present := annotations[metal3api.HardwareDetailsAnnotation]; present {
+		delete(host.Annotations, metal3api.HardwareDetailsAnnotation)
 		err := r.Update(context.TODO(), host)
 		if err != nil {
 			return updated, errors.Wrap(err, "Could not update removing hardwaredetails annotation")
@@ -427,7 +424,7 @@ func getRebootAnnotationArguments(annotation string, info *reconcileInfo) (resul
 
 // isRebootAnnotation returns true if the provided annotation is a reboot annotation (either suffixed or not)
 func isRebootAnnotation(annotation string) bool {
-	return strings.HasPrefix(annotation, rebootAnnotationPrefix+"/") || annotation == rebootAnnotationPrefix
+	return strings.HasPrefix(annotation, metal3api.RebootAnnotationPrefix+"/") || annotation == metal3api.RebootAnnotationPrefix
 }
 
 // clearRebootAnnotations deletes all reboot annotations exist on the provided host
@@ -446,7 +443,7 @@ func clearRebootAnnotations(host *metal3api.BareMetalHost) (dirty bool) {
 // which means we don't inspect even in Inspecting state
 func inspectionDisabled(host *metal3api.BareMetalHost) bool {
 	annotations := host.GetAnnotations()
-	return annotations[inspectAnnotationPrefix] == "disabled"
+	return annotations[metal3api.InspectAnnotationPrefix] == "disabled"
 }
 
 // hasInspectAnnotation checks for existence of inspect.metal3.io annotation
@@ -454,7 +451,7 @@ func inspectionDisabled(host *metal3api.BareMetalHost) bool {
 func hasInspectAnnotation(host *metal3api.BareMetalHost) bool {
 	annotations := host.GetAnnotations()
 	if annotations != nil {
-		if expect, ok := annotations[inspectAnnotationPrefix]; ok && expect != "disabled" {
+		if expect, ok := annotations[metal3api.InspectAnnotationPrefix]; ok && expect != "disabled" {
 			return true
 		}
 	}
@@ -956,7 +953,7 @@ func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner,
 
 		// Delete inspect annotation if exists
 		if hasInspectAnnotation(info.host) {
-			delete(info.host.Annotations, inspectAnnotationPrefix)
+			delete(info.host.Annotations, metal3api.InspectAnnotationPrefix)
 			dirty = true
 		}
 
@@ -1318,8 +1315,8 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	desiredPowerOnState := info.host.Spec.Online
 
 	if !info.host.Status.PoweredOn {
-		if _, suffixlessAnnotationExists := info.host.Annotations[rebootAnnotationPrefix]; suffixlessAnnotationExists {
-			delete(info.host.Annotations, rebootAnnotationPrefix)
+		if _, suffixlessAnnotationExists := info.host.Annotations[metal3api.RebootAnnotationPrefix]; suffixlessAnnotationExists {
+			delete(info.host.Annotations, metal3api.RebootAnnotationPrefix)
 
 			if err = r.Update(context.TODO(), info.host); err != nil {
 				return actionError{errors.Wrap(err, "failed to remove reboot annotation from host")}
@@ -1591,11 +1588,11 @@ func (r *BareMetalHostReconciler) getHostStatusFromAnnotation(host *metal3api.Ba
 // extract HardwareDetails from annotation if present
 func (r *BareMetalHostReconciler) getHardwareDetailsFromAnnotation(host *metal3api.BareMetalHost) (*metal3api.HardwareDetails, error) {
 	annotations := host.GetAnnotations()
-	if annotations[hardwareDetailsAnnotation] == "" {
+	if annotations[metal3api.HardwareDetailsAnnotation] == "" {
 		return nil, nil
 	}
 	objHardwareDetails := &metal3api.HardwareDetails{}
-	decoder := json.NewDecoder(strings.NewReader(annotations[hardwareDetailsAnnotation]))
+	decoder := json.NewDecoder(strings.NewReader(annotations[metal3api.HardwareDetailsAnnotation]))
 	decoder.DisallowUnknownFields()
 	if err := decoder.Decode(objHardwareDetails); err != nil {
 		return nil, err


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index 72ce030c..e5dd5aa3 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -640,7 +640,7 @@ func TestValidateCreate(t *testing.T) {
 					Name:      "test",
 					Namespace: "test-namespace",
 					Annotations: map[string]string{
-						InspectAnnotation:         "disabled",
+						InspectAnnotationPrefix:   "disabled",
 						HardwareDetailsAnnotation: `{"INVALIDField":{"manufacturer":"QEMU","productName":"Standard PC (Q35 + ICH9, 2009)","serialNumber":""},"firmware":{"bios":{"date":"","vendor":"","version":""}},"ramMebibytes":4096,"nics":[{"name":"eth0","model":"0x1af4 0x0001","mac":"00:b7:8b:bb:3d:f6","ip":"172.22.0.64","speedGbps":0,"vlanId":0,"pxe":true}],"storage":[{"name":"/dev/sda","rotational":true,"sizeBytes":53687091200,"vendor":"QEMU","model":"QEMU HARDDISK","serialNumber":"drive-scsi0-0-0-0","hctl":"6:0:0:0"}],"cpu":{"arch":"x86_64","model":"Intel Xeon E3-12xx v2 (IvyBridge)","clockMegahertz":2494.224,"flags":["foo"],"count":4},"hostname":"hwdAnnotation-0"}`,
 					},
 				},
@@ -655,7 +655,7 @@ func TestValidateCreate(t *testing.T) {
 					Name:      "test",
 					Namespace: "test-namespace",
 					Annotations: map[string]string{
-						InspectAnnotation:         "disabled",
+						InspectAnnotationPrefix:   "disabled",
 						HardwareDetailsAnnotation: `{"INVALIDField":{"manufacturer":"QEMU","productName":"Standard PC (Q35 + ICH9, 2009)","serialNumber":""},"firmware":{"bios":{"date":"","vendor":"","version":""}},"ramMebibytes":4096,"nics":[{"name":"eth0","model":"0x1af4 0x0001","mac":"00:b7:8b:bb:3d:f6","ip":"172.22.0.64","speedGbps":0,"vlanId":0,"pxe":true}],"storage":[{"name":"/dev/sda","rotational":true,"sizeBytes":53687091200,"vendor":"QEMU","model":"QEMU HARDDISK","serialNumber":"drive-scsi0-0-0-0","hctl":"6:0:0:0"}],"cpu":{"arch":"x86_64","model":"Intel Xeon E3-12xx v2 (IvyBridge)","clockMegahertz":2494.224,"flags":["foo"],"count":4},"hostname":"hwdAnnotation-0"`,
 					},
 				},
@@ -671,7 +671,7 @@ func TestValidateCreate(t *testing.T) {
 					Name:      "test",
 					Namespace: "test-namespace",
 					Annotations: map[string]string{
-						InspectAnnotation: "enabled",
+						InspectAnnotationPrefix: "enabled",
 					},
 				},
 			},
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index e6f05a1e..c29c7aa7 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -247,14 +247,14 @@ func waitForProvisioningState(t *testing.T, r *BareMetalHostReconciler, host *me
 func TestHardwareDetails_EmptyStatus(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = map[string]string{
-		hardwareDetailsAnnotation: hwdAnnotation,
+		metal3api.HardwareDetailsAnnotation: hwdAnnotation,
 	}
 
 	r := newTestReconciler(host)
 
 	tryReconcile(t, r, host,
 		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
-			_, found := host.Annotations[hardwareDetailsAnnotation]
+			_, found := host.Annotations[metal3api.HardwareDetailsAnnotation]
 			if host.Status.HardwareDetails != nil && host.Status.HardwareDetails.Hostname == "hwdAnnotation-0" && !found {
 				return true
 			}
@@ -268,7 +268,7 @@ func TestHardwareDetails_EmptyStatus(t *testing.T) {
 func TestHardwareDetails_StatusPresent(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = map[string]string{
-		hardwareDetailsAnnotation: hwdAnnotation,
+		metal3api.HardwareDetailsAnnotation: hwdAnnotation,
 	}
 	time := metav1.Now()
 	host.Status.LastUpdated = &time
@@ -280,7 +280,7 @@ func TestHardwareDetails_StatusPresent(t *testing.T) {
 
 	tryReconcile(t, r, host,
 		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
-			_, found := host.Annotations[hardwareDetailsAnnotation]
+			_, found := host.Annotations[metal3api.HardwareDetailsAnnotation]
 			if host.Status.HardwareDetails != nil && host.Status.HardwareDetails.Hostname == "existinghost" && !found {
 				return true
 			}
@@ -295,8 +295,8 @@ func TestHardwareDetails_StatusPresent(t *testing.T) {
 func TestHardwareDetails_StatusPresentInspectDisabled(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = map[string]string{
-		inspectAnnotationPrefix:   "disabled",
-		hardwareDetailsAnnotation: hwdAnnotation,
+		metal3api.InspectAnnotationPrefix:   "disabled",
+		metal3api.HardwareDetailsAnnotation: hwdAnnotation,
 	}
 	time := metav1.Now()
 	host.Status.LastUpdated = &time
@@ -308,7 +308,7 @@ func TestHardwareDetails_StatusPresentInspectDisabled(t *testing.T) {
 
 	tryReconcile(t, r, host,
 		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
-			_, found := host.Annotations[hardwareDetailsAnnotation]
+			_, found := host.Annotations[metal3api.HardwareDetailsAnnotation]
 			if host.Status.HardwareDetails != nil && host.Status.HardwareDetails.Hostname == "hwdAnnotation-0" && !found {
 				return true
 			}
@@ -323,8 +323,8 @@ func TestHardwareDetails_Invalid(t *testing.T) {
 	host := newDefaultHost(t)
 	badAnnotation := fmt.Sprintf("{\"hardware\": %s}", hwdAnnotation)
 	host.Annotations = map[string]string{
-		inspectAnnotationPrefix:   "disabled",
-		hardwareDetailsAnnotation: badAnnotation,
+		metal3api.InspectAnnotationPrefix:   "disabled",
+		metal3api.HardwareDetailsAnnotation: badAnnotation,
 	}
 	time := metav1.Now()
 	host.Status.LastUpdated = &time
@@ -487,7 +487,7 @@ func TestPause(t *testing.T) {
 func TestInspectDisabled(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = map[string]string{
-		inspectAnnotationPrefix: "disabled",
+		metal3api.InspectAnnotationPrefix: "disabled",
 	}
 	r := newTestReconciler(host)
 	waitForProvisioningState(t, r, host, metal3api.StatePreparing)
@@ -589,7 +589,7 @@ func TestInspectionDisabledAnnotation(t *testing.T) {
 
 	assert.False(t, inspectionDisabled(host))
 
-	host.Annotations[inspectAnnotationPrefix] = "disabled"
+	host.Annotations[metal3api.InspectAnnotationPrefix] = "disabled"
 	assert.True(t, inspectionDisabled(host))
 }
 
@@ -615,29 +615,29 @@ func TestHasRebootAnnotation(t *testing.T) {
 		{
 			Scenario: "Simple with empty value",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix: "",
+				metal3api.RebootAnnotationPrefix: "",
 			},
 			expectedReboot: true,
 		},
 		{
 			Scenario: "Suffixed with empty value",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "",
+				metal3api.RebootAnnotationPrefix + "/foo": "",
 			},
 			expectedReboot: true,
 		},
 		{
 			Scenario: "Two suffixed with empty value",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "",
-				rebootAnnotationPrefix + "/bar": "",
+				metal3api.RebootAnnotationPrefix + "/foo": "",
+				metal3api.RebootAnnotationPrefix + "/bar": "",
 			},
 			expectedReboot: true,
 		},
 		{
 			Scenario: "Suffixed with soft reboot",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "{\"mode\": \"soft\"}",
+				metal3api.RebootAnnotationPrefix + "/foo": "{\"mode\": \"soft\"}",
 			},
 			expectedReboot: true,
 			expectedMode:   metal3api.RebootModeSoft,
@@ -645,7 +645,7 @@ func TestHasRebootAnnotation(t *testing.T) {
 		{
 			Scenario: "Suffixed with hard reboot",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "{\"mode\": \"hard\"}",
+				metal3api.RebootAnnotationPrefix + "/foo": "{\"mode\": \"hard\"}",
 			},
 			expectedReboot: true,
 			expectedMode:   metal3api.RebootModeHard,
@@ -653,15 +653,15 @@ func TestHasRebootAnnotation(t *testing.T) {
 		{
 			Scenario: "Suffixed with bad JSON",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "{\"bad\"= \"json\"]",
+				metal3api.RebootAnnotationPrefix + "/foo": "{\"bad\"= \"json\"]",
 			},
 			expectedReboot: true,
 		},
 		{
 			Scenario: "Two suffixed with different values",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "{\"mode\": \"hard\"}",
-				rebootAnnotationPrefix + "/bar": "{\"mode\": \"soft\"}",
+				metal3api.RebootAnnotationPrefix + "/foo": "{\"mode\": \"hard\"}",
+				metal3api.RebootAnnotationPrefix + "/bar": "{\"mode\": \"soft\"}",
 			},
 			expectedReboot: true,
 			expectedMode:   metal3api.RebootModeHard,
@@ -669,7 +669,7 @@ func TestHasRebootAnnotation(t *testing.T) {
 		{
 			Scenario: "Suffixed with force",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "{\"force\": true}",
+				metal3api.RebootAnnotationPrefix + "/foo": "{\"force\": true}",
 			},
 			expectForce:    true,
 			expectedReboot: true,
@@ -677,7 +677,7 @@ func TestHasRebootAnnotation(t *testing.T) {
 		{
 			Scenario: "Expect force",
 			Annotations: map[string]string{
-				rebootAnnotationPrefix + "/foo": "{\"mode\": \"hard\"}",
+				metal3api.RebootAnnotationPrefix + "/foo": "{\"mode\": \"hard\"}",
 			},
 			expectForce:    true,
 			expectedReboot: false,
@@ -706,7 +706,7 @@ func TestHasRebootAnnotation(t *testing.T) {
 func TestRebootWithSuffixlessAnnotation(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = make(map[string]string)
-	host.Annotations[rebootAnnotationPrefix] = ""
+	host.Annotations[metal3api.RebootAnnotationPrefix] = ""
 	host.Status.PoweredOn = true
 	host.Status.Provisioning.State = metal3api.StateProvisioned
 	host.Spec.Online = true
@@ -728,7 +728,7 @@ func TestRebootWithSuffixlessAnnotation(t *testing.T) {
 
 	tryReconcile(t, r, host,
 		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
-			if _, exists := host.Annotations[rebootAnnotationPrefix]; exists {
+			if _, exists := host.Annotations[metal3api.RebootAnnotationPrefix]; exists {
 				return false
 			}
 
@@ -764,7 +764,7 @@ func TestRebootWithSuffixlessAnnotation(t *testing.T) {
 func TestRebootWithSuffixedAnnotation(t *testing.T) {
 	host := newDefaultHost(t)
 	host.Annotations = make(map[string]string)
-	annotation := rebootAnnotationPrefix + "/foo"
+	annotation := metal3api.RebootAnnotationPrefix + "/foo"
 	host.Annotations[annotation] = ""
 	host.Status.PoweredOn = true
 	host.Status.Provisioning.State = metal3api.StateProvisioned
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 5fd0d546..5e29986a 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -1245,7 +1245,7 @@ func (hb *hostBuilder) DisableInspection() *hostBuilder {
 	if hb.Annotations == nil {
 		hb.Annotations = make(map[string]string, 1)
 	}
-	hb.Annotations[inspectAnnotationPrefix] = "disabled"
+	hb.Annotations[metal3api.InspectAnnotationPrefix] = "disabled"
 	return hb
 }