Problem
The BuildJobPlan() function in pkg/api/validator/v1/job_plan_internal.go reads from the calling process environment via os.Getenv() without documenting this behavior or providing parameters to control it:
// pkg/api/validator/v1/job_plan_internal.go:86-92
if override := os.Getenv("AICR_VALIDATOR_IMAGE_REGISTRY"); override != "" {
env = append(env, corev1.EnvVar{
Name: "AICR_VALIDATOR_IMAGE_REGISTRY",
Value: override,
})
}
if tag := os.Getenv("AICR_VALIDATOR_IMAGE_TAG"); tag != "" {
env = append(env, corev1.EnvVar{
Name: "AICR_VALIDATOR_IMAGE_TAG",
Value: tag,
})
}
Impact:
- External controllers calling
BuildJobPlan() have hidden dependencies on their process environment
- Job output varies based on caller's environment variables without explicit control
- Public API contract is unclear about this behavior
- Hard to test reliably
Origin:
This behavior existed in main:deployer.go:256 before the job planning API refactoring. The refactoring surfaced it in a public API, making the issue more visible.
Proposed Solutions
Option 1: Document (Minimum Fix)
Add godoc to BuildJobPlan() explaining the environment variable dependency:
// BuildJobPlan creates a JobPlan from a validator entry.
//
// Environment Variables:
// This function reads from the calling process environment:
// - AICR_VALIDATOR_IMAGE_REGISTRY: Overrides image registry for all validators
// - AICR_VALIDATOR_IMAGE_TAG: Overrides image tag for all validators
// These values are forwarded to validator containers. External controllers should
// be aware that their process environment will affect the generated JobPlan.
func BuildJobPlan(...) JobPlan {
Pros:
- ✅ Non-breaking change
- ✅ Makes behavior explicit
- ✅ Quick to implement
Cons:
- ⚠️ Doesn't give callers control
- ⚠️ Still couples API to process environment
Option 2: Parameterize (Better Fix)
Add explicit parameters to control these values:
func BuildJobPlan(
entry ValidatorEntry,
runID string,
namespace string,
version string,
commit string,
serviceAccount string,
imagePullSecrets []string,
tolerations []corev1.Toleration,
nodeSelector map[string]string,
imageRegistryOverride string, // New parameter
imageTagOverride string, // New parameter
) JobPlan {
// Use parameters instead of os.Getenv()
if imageRegistryOverride != "" {
env = append(env, corev1.EnvVar{
Name: "AICR_VALIDATOR_IMAGE_REGISTRY",
Value: imageRegistryOverride,
})
}
// ...
}
Pros:
- ✅ Explicit in function signature
- ✅ Caller has full control
- ✅ Easy to test
- ✅ No hidden dependencies
Cons:
- ⚠️ Breaking API change
- ⚠️ Requires updating all callers
References
Acceptance Criteria
Problem
The
BuildJobPlan()function inpkg/api/validator/v1/job_plan_internal.goreads from the calling process environment viaos.Getenv()without documenting this behavior or providing parameters to control it:Impact:
BuildJobPlan()have hidden dependencies on their process environmentOrigin:
This behavior existed in main:deployer.go:256 before the job planning API refactoring. The refactoring surfaced it in a public API, making the issue more visible.
Proposed Solutions
Option 1: Document (Minimum Fix)
Add godoc to
BuildJobPlan()explaining the environment variable dependency:Pros:
Cons:
Option 2: Parameterize (Better Fix)
Add explicit parameters to control these values:
Pros:
Cons:
References
Acceptance Criteria
os.Getenv()