Repository navigation
fix(api): validate OptimizationJob parameter names - #4062
mimosapudical wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@andreyvelich This is the follow-up to the Parameter.Name validation discussion in #3828. Thanks! |
saivedant169
left a comment
saivedant169
left a comment
There was a problem hiding this comment.
Ran this locally at f74b781. The specs pass on the 1.37 envtest, and if I drop the pattern from the CRD the slash case fails, so the test is doing its job. CRDs regenerate with no diff, lint and KAL are clean. Couple of small things inline.
0671455 to
23a4324
Compare
|
@saivedant169 Addressed all review comments in the latest commit, including the additional validation cases, clearer GoDoc, and the fixed test job name. Could you please take another look when you have a chance? |
|
looks good, thanks for turning it around so fast |
|
@akshaychitneni @robert-bell The latest changes have been reviewed and all feedback has been addressed. Would one of you mind taking a look and adding |
robert-bell
left a comment
robert-bell
left a comment
There was a problem hiding this comment.
Thanks @mimosapudical! Just a small nit but otherwise looks good. Ptal.
| "github.com/kubeflow/trainer/v2/test/integration/framework" | ||
| ) | ||
|
|
||
| func makeOptimizationJob(namespace, name, parameterName string) *trainer.OptimizationJob { |
There was a problem hiding this comment.
Could we create a wrapper type in pkg/util/testing please?
trainer/pkg/util/testing/wrapper.go
Line 41 in e2a9fbb
There was a problem hiding this comment.
Could we create a wrapper type in pkg/util/testing please?
trainer/pkg/util/testing/wrapper.go
Line 41 in e2a9fbb
?
Done — I added an OptimizationJobWrapper in pkg/util/testing/wrapper.go and updated the integration test to use it instead of the local helper. Thanks! @robert-bell
|
/ok-to-test |
|
@mimosapudical can you please sign your commits? |
3936e58 to
1e5b351
Compare
All commits are now signed and verified. The force-push only rewrote the commits to add signatures; there are no code changes. Could you please re-add |
|
@robert-bell All commits are now signed and the requested changes are unchanged. Could you re-add /lgtm when you have a chance? Thanks! |
|
Thanks @mimosapudical! /lgtm |
|
@mimosapudical the DCO check is still failing. Can you please check your commits are signed? There's instructions on the DCO action. Thanks! |
1e5b351 to
eb7a9dc
Compare
|
New changes are detected. LGTM label has been removed. |
|
/milestone v2.4 |
|
@mimosapudical could you please rebase and address conflicts? |
Retain upstream OptimizationJob webhook cases and generated API changes while adding parameter name admission validation and regression coverage. Signed-off-by: mimosapudical <153621177+mimosapudical@users.noreply.github.com>
7c20b0e to
f625380
Compare
What this PR does / why we need it:
Adds admission-time validation for
OptimizationJob.spec.parameters[].name.Parameter.Nameis later used when generating environment variable names. Values such asweight/decayorn layerscurrently passOptimizationJobadmission but can fail later duringTrainJobcreation because they produce invalid environment variable names.This PR adds the following kubebuilder validation pattern:
This allows names such as
learning_rate,_lr, andbatch_size2, while rejecting invalid names such asweight/decay,n layers,1learning_rate, andlearning-rate.It also:
This is a follow-up to #3828, where this validation was intentionally split into a separate PR.
Which issue(s) this PR fixes:
N/A — follow-up to #3828.
Checklist: