Update error msgs for OSImageURL validation: #7769

jacobweinstock · 2024-03-02T00:51:18Z

Issue #, if available:

Description of changes:
As the error message is provided to Users, including the expected Kubernetes version format to the error message allows to User to clearly understand how to resolve the validation error.

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.

As the error message is provided to Users, including the expected Kubernetes version format to the error message allows to User to clearly understand how to resolve the validation error. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

codecov · 2024-03-04T16:02:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.64%. Comparing base (4583834) to head (f6a54f5).
Report is 257 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7769      +/-   ##
==========================================
+ Coverage   73.48%   73.64%   +0.15%     
==========================================
  Files         579      588       +9     
  Lines       36357    37207     +850     
==========================================
+ Hits        26718    27400     +682     
- Misses       7875     8015     +140     
- Partials     1764     1792      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jacobweinstock · 2024-03-04T16:18:20Z

/approve

eks-distro-bot · 2024-03-04T16:18:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jacobweinstock

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:

~~OWNERS~~ [jacobweinstock]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rahulbabu95 · 2024-03-05T01:37:46Z

pkg/providers/tinkerbell/validate.go

+}
+
+// deduplicate returns a new slice with duplicates values removed.
+func deduplicate(s []string) []string {


wondering if this is needed, since we have the control over the input and this is not coming from user?

the control we have over the input is "business" level logic, this function is the "application" level logic. Coupling those together creates for us future issues when business logic changes. I don't recommend we couple them.

rahulbabu95 · 2024-03-05T01:47:46Z

pkg/providers/tinkerbell/validate.go

+	}
+
+	seps := remove(".", deduplicate(separators))
+	for idx, sep := range seps {


can we simplify this logic to just parsing in the version and concatenating the seps to list out all accepted versions in a single for loop

Suggested change

for idx, sep := range seps {

version := strings.Split(v, ".")

result := []string{v}

major := version[0]

minor := version[1]

for idx, sep := range seps {

elem := fmt.Sprintf("%s%s%s",major,sep,minor)

if idx == len(seps)-1 {

result = append(result, "or")

} else if idx != len(seps)-2 {

elem = elem + ","

}

result = append(result, elem)

}

return strings.Join(result, " ")

}

this changes the function to only support a single . as a separator. In my opinion, this more generic implementation is basic, flexible, and simple enough to be future proof. I'm not convinced of the value of the trade off of a few less lines of code.

rahulbabu95 · 2024-03-05T01:49:36Z

pkg/providers/tinkerbell/validate.go

+	result := []string{}
+	seen := make(map[string]struct{})
+	for _, val := range s {
+		val := strings.ToLower(val)


since these are all symbols we might not need this too!

this function doesnt limit the type of strings in the slice. the values could be characters. coupling the inputs to our business logic creates a rigid implementation.

rahulbabu95 · 2024-03-05T01:51:12Z

pkg/providers/tinkerbell/validate_test.go

+		{"1.27", []string{"."}, "1.27"},
+		{"1.28", []string{".", "."}, "1.28"},
+		{"1.29", []string{"-", "-", ""}, "1.29, 1-29 or 129"},
+		{"1.29.1", []string{"-", "_", "_", ""}, "1.29.1, 1-29-1, 1_29_1 or 1291"},


we don't support patch version on our spec, so we don't have to be covering this. hence a simpler approach might work?

what versions we support is business level logic. Coupling this with application logic will cause us future maintainability issues in the future. I don't recommend we couple them.

mitalipaygude · 2024-03-19T21:05:21Z

pkg/providers/tinkerbell/validate.go

@@ -150,8 +150,8 @@ func validateK8sVersionInOSImageURLs(spec *ClusterSpec) error {
 		}

 		if !containsK8sVersion(spec.ControlPlaneMachineConfig().Spec.OSImageURL, string(spec.Cluster.Spec.KubernetesVersion)) {
-			return fmt.Errorf("missing kube version from control plane machine config OSImageURL: url=%v, version=%v",
-				spec.ControlPlaneMachineConfig().Spec.OSImageURL, spec.Cluster.Spec.KubernetesVersion)
+			return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %v: url=%v, version=%v",


Suggested change

return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %v: url=%v, version=%v",

return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %s: url=%v, version=%v",

jacobweinstock requested a review from rahulbabu95 March 2, 2024 00:51

eks-distro-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 2, 2024

jacobweinstock force-pushed the update-OSImageURL-validation branch 2 times, most recently from 566e170 to cbd1c75 Compare March 2, 2024 01:25

eks-distro-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 2, 2024

jacobweinstock force-pushed the update-OSImageURL-validation branch 4 times, most recently from 482e07d to 4abcacf Compare March 2, 2024 02:31

eks-distro-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2024

jacobweinstock force-pushed the update-OSImageURL-validation branch 4 times, most recently from c1f8ed0 to 6aaa4d9 Compare March 4, 2024 15:49

jacobweinstock force-pushed the update-OSImageURL-validation branch from 6aaa4d9 to f6a54f5 Compare March 4, 2024 15:56

eks-distro-bot added the approved label Mar 4, 2024

rahulbabu95 reviewed Mar 5, 2024

View reviewed changes

mitalipaygude reviewed Mar 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update error msgs for OSImageURL validation: #7769

Update error msgs for OSImageURL validation: #7769

jacobweinstock commented Mar 2, 2024

codecov bot commented Mar 4, 2024 •

edited

Loading

jacobweinstock commented Mar 4, 2024

eks-distro-bot commented Mar 4, 2024

rahulbabu95 Mar 5, 2024

jacobweinstock Mar 5, 2024

rahulbabu95 Mar 5, 2024

jacobweinstock Mar 5, 2024

rahulbabu95 Mar 5, 2024

jacobweinstock Mar 5, 2024

rahulbabu95 Mar 5, 2024

jacobweinstock Mar 5, 2024

mitalipaygude Mar 19, 2024

-	for idx, sep := range seps {
+        version := strings.Split(v, ".")
+        result := []string{v}
+        major := version[0]
+        minor := version[1]
+	for idx, sep := range seps {
+	       elem := fmt.Sprintf("%s%s%s",major,sep,minor)
+	       if idx == len(seps)-1 {
+	       result = append(result, "or")
+	       } else if idx != len(seps)-2 {
+			elem = elem + ","
+		}
+		result = append(result, elem)
+	}
+	return strings.Join(result, " ")
+}

	return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %v: url=%v, version=%v",
	return fmt.Errorf("missing kube version from control plane machine config OSImageURL, the image url must include %s: url=%v, version=%v",

Update error msgs for OSImageURL validation: #7769

Are you sure you want to change the base?

Update error msgs for OSImageURL validation: #7769

Conversation

jacobweinstock commented Mar 2, 2024

codecov bot commented Mar 4, 2024 • edited Loading

Codecov Report

jacobweinstock commented Mar 4, 2024

eks-distro-bot commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2024 •

edited

Loading