Flesh out tests for types.go, parser.go; add validation_test.go, update validation.go; add canonical formatting for cpu and memory resources #1

maxwellgagnon · 2025-10-10T19:52:40Z

Breakdown of changes to each file:

parser.go

Add CPU and Memory normalization to relevant functions
Add relevant functions for doing the CPU and Memory normalization
Move normalizeSSHKeys from validation.go to parser.go to be inline with CPU and Memory normalization code

types.go

add CPURaw and MemoryRaw to ResourceConfig() to house pre-processed resource information
Update expected cpu and memory values to reflect canonical data formatting
Update CPU and Memory functions to reflect canonical data formatting
Fix slicing vulnerability in GetSSHKeysSlice() and VolumeMounts()
Add negative value check for GPU()

validation.go

Overhaul for validation of parsed data structure, improving breadth of testing

parser_test.go

Increase scope of previous test, and add new relevant tests for parser.go

types_test.go

Increase scope of previous test, and add new relevant tests for types.go

validation_test.go

New file, add tests for validation.go

renderer_test.go

Update CPU and Memory to reflect canonical data formatting

statefulset.yaml

Update CPU and Memory to reflect canonical data formatting

generate.go

Update CPU and Memory to reflect canonical data formatting

example_test.go

Removed redundant tests (types_test.go and parser_test.go)
Updated expected CPU information to match canonical millicores formatting
Changed fatal error, to graceful error

…lization

…matting

eywalker

Very nice changes! Generally I like the updates.

Only part to be discussed and considered is the consistency in the internal representation. I'm not a huge fan of a structure having two public fields (i.e. CPU and CPURaw) that are supposed to be consistent with each other. This can lead to possible inconsistency. We should consider a way to only have one field that'd act as the source of truth and render other views as methods.

eywalker · 2025-10-11T02:58:18Z

internal/config/parser.go

 	}

+	if err := normalizeCPU(&globalConfig.Resources); err != nil {
+		return nil, fmt.Errorf("normalize cpu: %w", err)


Let's make the error message more descriptive e.g. Failed to normalize CPU resource %v: %w and printout the problematic resource string.

eywalker · 2025-10-11T03:00:56Z

internal/config/types.go

-			Memory:  "8Gi",  // Default Memory
-			Storage: "20Gi", // Default Storage
-			GPU:     0,      // Default GPU
+			CPU:     2000,     // Default CPU (2 cores)


I actually think it'd be better for us to accept resource specification that's valid for Kubernetes. https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-units-in-kubernetes

eywalker · 2025-10-11T03:02:34Z

internal/config/types.go


-// Memory returns the memory resource allocation as a string suitable for Kubernetes manifests.
+// Memory returns the canonical memory quantity formatted for Kubernetes.
+// Assumes Resources.Memory has been normalized to Mi (mebibytes).


Why normalize to one specific unit?

eywalker · 2025-10-11T03:12:49Z

cmd/devenv/generate.go

+			}
+			fmt.Printf(" Memory=%s", memStr)
+		}
+		fmt.Println()


It's a bit redundant now. Let's simplify.

Suggested change

fmt.Println()

hasCPU := cpuStr != "0"

hasMem := memStr != ""

if hasCPU || hasMem {

var parts []string

if hasCPU {

parts = append(parts, fmt.Sprintf("CPU=%s", cpuStr))

}

if hasMem {

parts = append(parts, fmt.Sprintf("Memory=%s", memStr))

}

fmt.Printf(" Resources: %s\n", strings.Join(parts, ", "))

}

eywalker · 2025-10-11T03:14:13Z

internal/config/example_test.go

 	sshKeys, err := cfg.GetSSHKeys()
 	if err != nil {
-		log.Fatal(err)
+		fmt.Println("error:", err)


Let's keep error behavior consistent and have if log.Fatal much like on line 16

eywalker · 2025-10-11T03:16:34Z

internal/config/parser.go

 	config.DeveloperDir = developerDir

+	if err := normalizeCPU(&config.Resources); err != nil {
+		return nil, fmt.Errorf("normalize cpu: %w", err)


Same as above: give more descriptive error message while showing the failed value.

eywalker · 2025-10-11T03:20:28Z

internal/config/types.go

-	GPU     int    `yaml:"gpu,omitempty" validate:"omitempty,min=0,max=8"` // Number of GPUs requested
+	CPURaw    any    `yaml:"cpu,omitempty" validate:"omitempty,k8s_cpu"` // Can be string or int
+	MemoryRaw any    `yaml:"memory,omitempty" validate:"omitempty,k8s_memory"`
+	CPU       int64  `yaml:"-"` // canonical


In this case, I'd prefer that we call it CPU and Memory for the original and CanonicalCPU and CanonicalMemory to distinguish processed and canonicalized values.

Also having an entirely separate public field runs the risk of duplicated source of truth. Consequently, you should consider making the field private and accessed via methods.

Rather than implementing CPU and Memory as an entirely separate and partly redundant fields, keep the original field of CPU and Memory as loaded from YAML but provide getCPU and getMemory that provides normalized numerical values of the CPU and Memory using your normalized[CPU|Memory] functions.

eywalker

I've more carefully stepped through the code and suggested more concrete changes to be made. Speficially, I'd prefer we stick to original value of supplied memory and CPU to be stored in Memory and CPU field, but provide getMemory and getCPU methods that would provide fully normalized numerical values.
I've also implemented a few improvments in the code to be made -- please refer to the comments and let me know if you have any questions!

eywalker · 2025-10-12T20:27:54Z

internal/config/example_test.go

 	// User ID: 2000
 	// SSH Keys: 1 configured
 }
-


These functions are specifically added to serve as usage examples and thus should not be removed even if equivalent feature is tested in real testing. Please restore them

eywalker · 2025-10-12T20:30:06Z

internal/config/parser.go

+		return nil, fmt.Errorf("normalize cpu: %w", err)
+	}
+
+	if err := normalizeMemory(&globalConfig.Resources); err != nil {


The interface into normalizeCPU and normalizeMemory is a bit awkward, specifically working with Resources. Either make them methods attached to the Resources struct or turn them to simply accept a string that would return normalized values back.

eywalker · 2025-10-12T20:30:45Z

internal/config/parser.go

 	// Step 6: Set developer directory and validate
 	userConfig.DeveloperDir = developerDir

+	// Step 7: Normalize flexible/raw fields → canonical representation


Same set of comments as in the earlier usage of normalizeCPU and normalizeMemory

eywalker · 2025-10-12T20:31:49Z

internal/config/parser.go

+// normalizeCPU reads r.CPURaw and sets r.CPU (millicores).
+// With strict policy, invalid or non-positive inputs propagate as zero with an error, or
+// you can choose to return ok=false and let the loader decide. Here I return an error.
+func normalizeCPU(r *ResourceConfig) error {


don't modify the ResourceConfig. Rather accept the CPURaw string value and simply return normalized numerical value.

eywalker · 2025-10-12T20:33:29Z

internal/config/parser.go

+
+// toMillicores converts a flexible CPU value to millicores.
+// Policy: syntactically valid but non-positive (<=0) values are INVALID → ok=false.
+func toMillicores(v any) (int64, bool) {


I think this function is performing too many things at once. Consider splitting this into 1) a function to parse any into a acceptable string and 2) something that takes the string input and provide normalized value readout numerically.

eywalker · 2025-10-12T20:38:13Z

internal/config/parser.go

+
+// toMi converts a flexible memory value into canonical Mi (mebibytes).
+// Strict policy: syntactically valid but non-positive (<= 0) values are INVALID (ok=false).
+func toMi(v any) (int64, bool) {


Extract utility functions like parseStringToMi into a separate go file.

eywalker · 2025-10-12T20:39:07Z

internal/config/parser.go

+// Returns (0,false) if value <= 0 or on invalid input.
+func parseStringToMi(s string) (int64, bool) {
+	// ---- Binary SI (powers of 1024) ----
+	if hasSuffixFold(s, "Ki") {


Reimplement this logic using map from suffix to factor so that you don't repeat identical logic for every possible suffix.

eywalker · 2025-10-12T20:43:56Z

internal/config/types.go

-	GPU     int    `yaml:"gpu,omitempty" validate:"omitempty,min=0,max=8"` // Number of GPUs requested
+	CPURaw    any    `yaml:"cpu,omitempty" validate:"omitempty,k8s_cpu"` // Can be string or int
+	MemoryRaw any    `yaml:"memory,omitempty" validate:"omitempty,k8s_memory"`
+	CPU       int64  `yaml:"-"` // canonical


Rather than implementing CPU and Memory as an entirely separate and partly redundant fields, keep the original field of CPU and Memory as loaded from YAML but provide getCPU and getMemory that provides normalized numerical values of the CPU and Memory using your normalized[CPU|Memory] functions.

eywalker · 2025-10-12T20:44:37Z

internal/config/types.go

-			Storage: "20Gi", // Default Storage
-			GPU:     0,      // Default GPU
+			CPU:     2000,     // Default CPU (2 cores)
+			Memory:  8 * 1024, // Default Memory (8Gi -> 8192Mi)


Revert back to using string values as the resource specification, and rather provide normalized numerical values as function outputs.

eywalker · 2025-10-12T20:45:45Z

internal/config/validation.go

-			}
-		}
-		return keys, nil
+	case int:


make use of composite case like done in uint cases

eywalker · 2025-10-12T20:50:24Z

internal/config/validation.go

 }
+
+// Small float guards for validator predicates (avoid NaN/Inf).
+func float32IsNaNOrInf(f float32) bool {


Use math.IsNaN() and math.IsInf() instead to detect NaN and Inf

maxwellgagnon added 2 commits October 9, 2025 15:34

Update tests for example, parser, and types; Add cpu and memory norma…

29dbfc9

…lization

Update validation.go, add validation_test.go

7133400

maxwellgagnon requested a review from eywalker October 10, 2025 19:53

maxwellgagnon closed this Oct 10, 2025

maxwellgagnon reopened this Oct 10, 2025

maxwellgagnon marked this pull request as draft October 10, 2025 19:59

maxwellgagnon removed the request for review from eywalker October 10, 2025 19:59

maxwellgagnon added 2 commits October 10, 2025 15:31

Update statefulset.yaml and generate.go to reflect cpu and memory for…

a562870

…matting

Update additional file for cpu/mem formatting

0430a93

maxwellgagnon requested a review from eywalker October 10, 2025 22:44

maxwellgagnon marked this pull request as ready for review October 10, 2025 22:44

eywalker requested changes Oct 11, 2025

View reviewed changes

eywalker requested changes Oct 12, 2025

View reviewed changes

-		fmt.Println()
+hasCPU := cpuStr != "0"
+hasMem := memStr != ""
+if hasCPU || hasMem {
+    var parts []string
+    if hasCPU {
+        parts = append(parts, fmt.Sprintf("CPU=%s", cpuStr))
+    }
+    if hasMem {
+        parts = append(parts, fmt.Sprintf("Memory=%s", memStr))
+    }
+    fmt.Printf("  Resources: %s\n", strings.Join(parts, ", "))
+}

Flesh out tests for types.go, parser.go; add validation_test.go, update validation.go; add canonical formatting for cpu and memory resources #1

Are you sure you want to change the base?

Flesh out tests for types.go, parser.go; add validation_test.go, update validation.go; add canonical formatting for cpu and memory resources #1

Uh oh!

Conversation

maxwellgagnon commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breakdown of changes to each file:

parser.go

types.go

validation.go

parser_test.go

types_test.go

validation_test.go

renderer_test.go

statefulset.yaml

generate.go

example_test.go

Uh oh!

eywalker left a comment

Choose a reason for hiding this comment

Uh oh!

eywalker Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eywalker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxwellgagnon commented Oct 10, 2025 •

edited

Loading

eywalker Oct 11, 2025 •

edited

Loading