Background
Several functions in pkg/k8s/deployer.go track references to cluster
resources (Secrets, ConfigMaps, PVCs) so that CheckResourcesArePresent
can fail fast if anything referenced by a function isn't on the
cluster. The current shape passes three independent sets as
out-parameters via pointer:
func ProcessEnvs(envs []fn.Env, referencedSecrets, referencedConfigMaps *sets.Set[string]) (...)
func ProcessVolumes(volumes []fn.Volume, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) (...)
func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps *sets.Set[string]) (...)
func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps *sets.Set[string]) (...)
func (d *Deployer) generateDeployment(f fn.Function, namespace string, daprInstalled bool, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) (...)
func CheckResourcesArePresent(ctx context.Context, namespace string, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string], referencedServiceAccount, imagePullSecret string) error
Most recently propagated to generateDeployment in #3671 to fix a
silent-validation gap on the first-deploy path. That PR is the right
fix for the bug; this issue captures the design cleanup that the PR
deliberately did not absorb.
Why this is worth cleaning up
Three layers of smell sit on top of each other:
1. Pointer-to-map is doubly indirect.
k8s.io/apimachinery/pkg/util/sets.Set[T] is map[T]Empty. Maps in
Go are reference types — passing a Set by value already lets the
callee mutate the same backing storage. The * adds a level of
indirection that buys nothing the callers actually use: nobody
reassigns the set, nobody distinguishes a nil pointer from an empty
set, and the only operations the callees invoke are Insert and
Has. Idiomatic Go here is sets.Set[T] by value.
2. The triple is structurally coupled but passed as three loose
parameters. Every signature that touches reference tracking repeats
referencedSecrets, referencedConfigMaps, referencedPVCs. That's a
cohesive triple — it deserves a name:
type References struct {
Secrets sets.Set[string]
ConfigMaps sets.Set[string]
PVCs sets.Set[string]
}
Once the triple has a type, every call site collapses from three
arguments to one.
3. Accumulator-by-out-param is C-style in a language with
multi-return. Two cleaner shapes are available:
// (a) multi-return — pure-function flavor:
func ProcessEnvs(envs []fn.Env) ([]corev1.EnvVar, []corev1.EnvFromSource, References, error)
// (b) receiver-as-tracker — OO flavor:
type Tracker struct { References }
func (t *Tracker) ProcessEnvs(envs []fn.Env) ([]corev1.EnvVar, []corev1.EnvFromSource, error)
Either reads better than threading three out-pointers through the call
chain. (b) also makes it harder to forget to wire one of the sets
through, which is exactly the bug class that #3671 fixed (the local
sets in generateDeployment were silently invisible to the caller).
Suggested approach
Pick one shape — (a) or (b) above — and apply it consistently.
Either change is mechanical:
- Define
References (and Tracker, if going with shape (b)) in a
shared spot, likely near the top of pkg/k8s/deployer.go.
- Update the affected functions to use it:
ProcessEnvs
ProcessVolumes
createEnvFromSource
createEnvVarSource
generateDeployment
CheckResourcesArePresent
- Update call sites in
Deploy() — both the create branch and the
update branch in pkg/k8s/deployer.go. The keda deployer inherits
via delegation, no separate work needed there.
- Update the unit tests in
pkg/k8s/deployer_test.go for the new
signatures. Integration tests should not need behavior changes.
- Confirm no behavior changes — this is purely a shape refactor.
Out of scope
Why this wasn't done in #3671
#3671 is a focused bug fix from a contributor. Asking them to absorb
a refactor of a pre-existing pattern would have widened scope unrelated
to the bug and gated a real user-facing fix on cleanup work. This issue
captures the cleanup as separable, picker-uppable work.
Good first issue?
Reasonable candidate — the change is mechanical, the affected
functions are localized to one file (plus the test file), and the
desired end state is clearly specified. The judgment call between
shapes (a) and (b) is the only design decision; either is acceptable.
Background
Several functions in
pkg/k8s/deployer.gotrack references to clusterresources (Secrets, ConfigMaps, PVCs) so that
CheckResourcesArePresentcan fail fast if anything referenced by a function isn't on the
cluster. The current shape passes three independent sets as
out-parameters via pointer:
Most recently propagated to
generateDeploymentin #3671 to fix asilent-validation gap on the first-deploy path. That PR is the right
fix for the bug; this issue captures the design cleanup that the PR
deliberately did not absorb.
Why this is worth cleaning up
Three layers of smell sit on top of each other:
1. Pointer-to-map is doubly indirect.
k8s.io/apimachinery/pkg/util/sets.Set[T]ismap[T]Empty. Maps inGo are reference types — passing a
Setby value already lets thecallee mutate the same backing storage. The
*adds a level ofindirection that buys nothing the callers actually use: nobody
reassigns the set, nobody distinguishes a nil pointer from an empty
set, and the only operations the callees invoke are
InsertandHas. Idiomatic Go here issets.Set[T]by value.2. The triple is structurally coupled but passed as three loose
parameters. Every signature that touches reference tracking repeats
referencedSecrets, referencedConfigMaps, referencedPVCs. That's acohesive triple — it deserves a name:
Once the triple has a type, every call site collapses from three
arguments to one.
3. Accumulator-by-out-param is C-style in a language with
multi-return. Two cleaner shapes are available:
Either reads better than threading three out-pointers through the call
chain. (b) also makes it harder to forget to wire one of the sets
through, which is exactly the bug class that #3671 fixed (the local
sets in
generateDeploymentwere silently invisible to the caller).Suggested approach
Pick one shape — (a) or (b) above — and apply it consistently.
Either change is mechanical:
References(andTracker, if going with shape (b)) in ashared spot, likely near the top of
pkg/k8s/deployer.go.ProcessEnvsProcessVolumescreateEnvFromSourcecreateEnvVarSourcegenerateDeploymentCheckResourcesArePresentDeploy()— both the create branch and theupdate branch in
pkg/k8s/deployer.go. The keda deployer inheritsvia delegation, no separate work needed there.
pkg/k8s/deployer_test.gofor the newsignatures. Integration tests should not need behavior changes.
Out of scope
use the k8s functions but don't define their own. Once the k8s side
is done, the rest follows.
Why this wasn't done in #3671
#3671 is a focused bug fix from a contributor. Asking them to absorb
a refactor of a pre-existing pattern would have widened scope unrelated
to the bug and gated a real user-facing fix on cleanup work. This issue
captures the cleanup as separable, picker-uppable work.
Good first issue?
Reasonable candidate — the change is mechanical, the affected
functions are localized to one file (plus the test file), and the
desired end state is clearly specified. The judgment call between
shapes (a) and (b) is the only design decision; either is acceptable.