fix: Modify validation to check Analysis args passed through RO spec #1215
Conversation
Signed-off-by: khhirani <[email protected]>
…to fix-valuefrom-analysistemplate
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1215 +/- ##
==========================================
+ Coverage 81.09% 81.14% +0.05%
==========================================
Files 106 106
Lines 9519 9531 +12
==========================================
+ Hits 7719 7734 +15
+ Misses 1269 1267 -2
+ Partials 531 530 -1
Continue to review full report at Codecov.
|
|
Thanks for fixing. |
| allErrs = append(allErrs, field.Invalid(fldPath, value, err.Error())) | ||
| return allErrs | ||
| } | ||
| err = analysisutil.ResolveArgs(flattenedTemplate.Spec.Args) |
There was a problem hiding this comment.
it seems this method can be removed as this is the only place to use it.
Signed-off-by: khhirani <[email protected]>
…hhirani/argo-rollouts into fix-analysistemplate-validation
Signed-off-by: khhirani <[email protected]>
|
@jessesuen I modified the existing tests to cover the change |
Signed-off-by: khhirani <[email protected]>
…to fix-analysistemplate-validation
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -636,6 +636,7 @@ func (c *rolloutContext) getReferencedRolloutAnalyses() (*[]validation.AnalysisT | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| templates.Args = blueGreen.PrePromotionAnalysis.Args | |||
There was a problem hiding this comment.
- Are you modifying the AnalysisTemplate which was pulled off the informer store (lister)? Objects received form the store should not be modified. See:
- It seems we are clobbering the entire arguments of the template with the Arguments of the Rollout. Does this change support default values (i.e. some arguments values are in the AnalysisTemplate, but others are supplied by the rollout).
There was a problem hiding this comment.
It seems we are clobbering the entire arguments of the template with the Arguments of the Rollout.
Can you elaborate on this? I thought we did this anyway when we created the AnalysisRun from the templates (i.e. combine the templates’ args with the AnalysisRunArguments passed through the RO spec) in the function NewAnalysisRunFromTemplates
There was a problem hiding this comment.
Are you modifying the AnalysisTemplate which was pulled off the informer store (lister)?
I don't believe so. I'm copying the AnalysisRunArguments from the Rollout Spec into a AnalysisTemplatesWithType object, which is a struct I created for validation. I pass the templates and the args from the RO spec into NewAnalysisRunFromTemplates for validation, which doesn't seem to modify the templates.
There was a problem hiding this comment.
Can you elaborate on this?
Can you confirm that this is the behavior?
kind: AnalysisTemplate
...
args:
- name: aaa
value: foo # default value
- name: bbb # mandatory supplied value
- name: ccc
value: bazArgs supplied by rollout (note that aaa is omitted)
kind: Rollout
...
args:
- name: bbb
value: "11111"
- name: ccc
value: "2222"The AnalysisRun should be created with
kind: AnalysisRun
args:
- name: aaa
value: foo
- name: bbb
value: "11111"
- name: ccc
value: "2222"There was a problem hiding this comment.
I don't believe so. I'm copying the AnalysisRunArguments from the Rollout Spec into a AnalysisTemplatesWithType object, which is a struct I created for validation
I took a closer look. You're right. I mistakenly thought it was returning AnalysisTemplates
There was a problem hiding this comment.
@jessesuen Can I merge this in then?
There was a problem hiding this comment.
My first concern is not valid, but can you answer my second concern?
There was a problem hiding this comment.
@jessesuen Yes, I confirm that the behavior in this comment is what happens.
…rgoproj#1215) * fix: Modify validation to check Analysis args passed through RO spec Signed-off-by: khhirani <[email protected]> Signed-off-by: caoyang001 <[email protected]>
…rgoproj#1215) * fix: Modify validation to check Analysis args passed through RO spec Signed-off-by: khhirani <[email protected]> Signed-off-by: caoyang001 <[email protected]>
…rgoproj#1215) * fix: Modify validation to check Analysis args passed through RO spec Signed-off-by: khhirani <[email protected]> Signed-off-by: caoyang001 <[email protected]>
…rgoproj#1215) * fix: Modify validation to check Analysis args passed through RO spec Signed-off-by: khhirani <[email protected]> Signed-off-by: caoyang001 <[email protected]>
…rgoproj#1215) * fix: Modify validation to check Analysis args passed through RO spec Signed-off-by: khhirani <[email protected]> Signed-off-by: caoyang001 <[email protected]>
Resolves #1208
Signed-off-by: khhirani [email protected]