Repository: metal3-io/baremetal-operator
Pull request #783 (2021-02-01 15:48:20)

Refactor capabilities parsing
The current approach won't scale when we have more capabilities.

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/783#" title="Author self-approved">dtantsur</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **stbenjam**
You can assign the PR to them by writing `/assign @stbenjam` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["stbenjam"]} -->
metal3-io-bot: @dtantsur: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
unit | 59590cb75afc2ac43d7e5d62452657d306b539c4 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/783/unit/1356268235116253184) | `/test unit`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @dtantsur: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
unit | 7f7feaac0aa9f1db8bf3899a4c511e74bae1973c | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/783/unit/1356269745246048260) | `/test unit`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 5fc067de..e51b42fa 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1042,27 +1042,15 @@ func buildCapabilitiesValue(ironicNode *nodes.Node, bootMode metal3v1alpha1.Boot
 		return
 	}
 
-	if !strings.Contains(existingCapabilities, "boot_mode") {
-		// No boot_mode is set, append and return
-		value = fmt.Sprintf("%s,%s", existingCapabilities, bootModeCapabilities[bootMode])
-		return
-	}
-
-	// The capabilities value has format "var1:val1,var2:val2". We
-	// know that boot_mode is there but not what value it has.  There
-	// are only 2 values, so we can replace the "wrong" string with
-	// the right one without fully parsing the string. We may want to
-	// change this later when we have more boot modes.
-
-	var fromMode metal3v1alpha1.BootMode
-	if bootMode == metal3v1alpha1.UEFI {
-		fromMode = metal3v1alpha1.Legacy
-	} else {
-		fromMode = metal3v1alpha1.UEFI
+	var filteredCapabilities []string
+	for _, item := range strings.Split(existingCapabilities, ",") {
+		if !strings.HasPrefix(item, "boot_mode:") {
+			filteredCapabilities = append(filteredCapabilities, item)
+		}
 	}
-	value = strings.ReplaceAll(existingCapabilities,
-		bootModeCapabilities[fromMode], bootModeCapabilities[bootMode])
+	filteredCapabilities = append(filteredCapabilities, bootModeCapabilities[bootMode])
 
+	value = strings.Join(filteredCapabilities, ",")
 	return
 }
 


Test patch:
diff --git a/pkg/provisioner/ironic/capabilities_test.go b/pkg/provisioner/ironic/capabilities_test.go
index e8881d26..e10d36d6 100644
--- a/pkg/provisioner/ironic/capabilities_test.go
+++ b/pkg/provisioner/ironic/capabilities_test.go
@@ -54,7 +54,7 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 				},
 			},
 			Mode:          metal3v1alpha1.UEFI,
-			ExpectedValue: "boot_mode:uefi,cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true",
+			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi",
 			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
@@ -65,7 +65,7 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 				},
 			},
 			Mode:          metal3v1alpha1.Legacy,
-			ExpectedValue: "boot_mode:bios,cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true",
+			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:bios",
 			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
@@ -76,7 +76,7 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 				},
 			},
 			Mode:          metal3v1alpha1.UEFI,
-			ExpectedValue: "boot_mode:uefi,cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true",
+			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi",
 			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
@@ -87,7 +87,7 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 				},
 			},
 			Mode:          metal3v1alpha1.Legacy,
-			ExpectedValue: "boot_mode:bios,cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true",
+			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:bios",
 			ExpectedOp:    nodes.ReplaceOp,
 		},
 	}