feat(local): make StorageClass, MetalLB, and HTTPSPort configurable#201
feat(local): make StorageClass, MetalLB, and HTTPSPort configurable#201viniciusdc merged 7 commits intomainfrom
Conversation
|
@dcmcand I'll review this. Can you resolve the conflicts when you have a chance? |
|
Hey @dcmcand — could you rebase this against main and resolve the conflicts when you get a chance? Three refactors landed after this was opened (#190, #208, #211) that changed the |
1f97d14 to
a88139c
Compare
|
@viniciusdc it is rebased now |
Six assertion lines were incorrectly pulled into the tests slice as table cases during the merge conflict resolution. Removed them and moved the net-new SupportsLocalGitOps check into the per-case assertion block alongside the existing zero-value field checks.
viniciusdc
left a comment
There was a problem hiding this comment.
Looked through the diff and nebari-dev/action-nebari-sandbox#15 — a few things worth discussing before merging.
Two questions:
pkg/provider/local/provider.go — the comment says "InfraSettings is called after Validate() has confirmed the config is parseable", but that ordering isn't enforced anywhere in the code. If someone calls InfraSettings without going through Validate (in a test, or a future codepath), they'll silently get defaults with no indication something went wrong. Is there a lifecycle guarantee that makes this safe? Or should we at least log the error instead of swallowing it?
Also on provider.go — why does UnmarshalProviderConfig take a context at all? For a pure config unmarshal context.Background() is fine, but if the function does any I/O the context matters and should be threaded through. If it's purely structural (the interface requires it), a quick comment there would save future readers the same question.
Two small things:
provider_test.go — the test covers metallb.enabled: false and the default (true implicitly), but not enabled: true explicitly. That's the case where the pointer is non-nil but *enabled == true — small gap in the pointer semantics coverage.
provider.go — cfg.ProviderConfig() is called without guarding against a nil cfg. Current callers never pass nil (and the "no local config block" test case passes a non-nil cfg with Providers: nil, which the rawCfg == nil guard handles correctly), but the function signature allows it. Worth a nil check at the top for robustness.
What looks good:
Enabled *bool for MetalLB is the right design — cleanly distinguishes "not set" (default true) from "explicitly false" without overloading the zero value. omitempty on a pointer works exactly as expected here.
The 8 table-driven test cases are solid: defaults, each field in isolation, full override, and the unmarshal error path. I also noticed the assertions verify the fields this PR doesn't touch (LoadBalancerAnnotations, KeycloakBasePath, SupportsLocalGitOps) haven't regressed — good discipline.
The integration evidence from nebari-dev/action-nebari-sandbox#15 is the best part: it removes the standard StorageClass workaround from create-cluster.sh — the one with the TODO pointing directly at this PR. That deletion is the cleanest possible proof this solves the underlying problem. The workaround only existed because this config field didn't. Full platform profile runs end-to-end against this branch in CI.
examples/local-config.yaml is clear and matches the actual struct. The k3s and custom MetalLB pool examples cover exactly the two cases that motivated this.
There was a problem hiding this comment.
Full platform profile runs end-to-end against this branch in CI nebari-dev/action-nebari-sandbox#15
Summary
storage_class,https_port, andmetallb(enabled + address_pool) as optional fields in the local provider configInfraSettings()reads these from config with current hardcoded values as defaultsstorage_class: local-pathandmetallb.enabled: falseCloses
Closes #200
What changed
Config (
pkg/provider/local/config.go)StorageClass,HTTPSPortfields toConfigMetalLBConfigstruct withEnabled(*boolto distinguish unset from false) andAddressPoolImplementation (
pkg/provider/local/provider.go)InfraSettings()now parses the local config block and overrides defaults for any set fieldsTests (
pkg/provider/local/provider_test.go)Examples (
examples/local-config.yaml)Example usage
k3s cluster:
Custom MetalLB pool (kind on non-default Docker network):
Test plan
go test ./pkg/provider/local/ -vpasses (8/8 test cases)golangci-lint run ./pkg/provider/local/passes