feat(validator): parameterize image override env vars#1028
Conversation
Signed-off-by: Christopher Haar <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Clean refactor — moves os.Getenv for AICR_VALIDATOR_IMAGE_REGISTRY / AICR_VALIDATOR_IMAGE_TAG from pkg/validator/job/deployer.go to the CLI boundary at pkg/cli/validate.go, threading the values through validationConfig → WithImage*Override options → Validator fields → NewDeployer. Pattern is consistent with the existing functional-option / field-on-Validator shape (mirrors WithNodeSelector, WithImagePullSecrets, etc.), and matches the CLAUDE.md guidance that internal packages shouldn't read from the process environment directly.
Tests look good: TestDeployJobImageTagOverrideForwarding is now hermetic (no t.Setenv), the new TestDeployJobImageRegistryOverrideForwarding covers both set and empty cases, and every in-tree NewDeployer call site is updated.
One non-blocking observation for a future PR: NewDeployer is up to 12 positional parameters now — at some point an options struct would be friendlier than continuing to extend the signature. Out of scope here.
LGTM.
2 participants
Summary
Parameterize
AICR_VALIDATOR_IMAGE_REGISTRYandAICR_VALIDATOR_IMAGE_TAGin theDeployerandValidatortypes, replacing hiddenos.Getenv()reads with explicit constructor/option parameters threaded from the CLI boundary.Motivation / Context
The
Deployer.DeployJob()method read image override env vars from the calling process environment without documenting or exposing control to callers. This created hidden dependencies, made behavior hard to test, and coupled internal APIs to the process environment. TheBuildJobPlan()public API was already parameterized, this change bringsDeployerinto alignment.Fixes: #954
Related: #906
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
os.Getenv()calls moved topkg/cli/validate.go(the CLI boundary), passed throughvalidationConfig→WithImageRegistryOverride/WithImageTagOverrideoptions →Validatorfields →NewDeployerconstructor →BuildJobPlan.NewDeployersignature gains two string parameters (imageRegistryOverride,imageTagOverride). This is a breaking change for direct callers ofNewDeployer, but the only in-tree caller (pkg/validator/validator.go) is updated in this PR.os.Getenvcalls inpkg/validator/catalog/catalog.goandvalidators/performance/inference_perf_constraint.goare intentionally untouched, those execute inside validator container pods where reading from the pod's own environment is correct.Testing
TestDeployJobImageTagOverrideForwardingupdated to use constructor parameter instead oft.SetenvTestDeployJobImageRegistryOverrideForwardingadded covering set/empty casesRisk Assessment
Rollout notes:
no external API change, behavior is identical when invoked via the CLI. External consumers of NewDeployer (if any exist out-of-tree) will need to add two string arguments.
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info