Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: verify analysis arguments name with those in the rollout #1071

Merged
merged 2 commits into from
Apr 14, 2021

Conversation

huikang
Copy link
Member

@huikang huikang commented Apr 8, 2021

  • return error if arguments do not match between rollout spec and analysis template

close #1052

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #1071 (6aecdf0) into master (37668f1) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1071      +/-   ##
==========================================
+ Coverage   81.00%   81.04%   +0.04%     
==========================================
  Files         103      103              
  Lines        9166     9186      +20     
==========================================
+ Hits         7425     7445      +20     
  Misses       1246     1246              
  Partials      495      495              
Impacted Files Coverage Δ
.../apis/rollouts/validation/validation_references.go 76.92% <100.00%> (+2.63%) ⬆️
rollout/sync.go 72.37% <0.00%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37668f1...6aecdf0. Read the comment docs.

- return error if arguments do not match between rollout spec and analysis template

Signed-off-by: Hui Kang <hui.kang@salesforce.com>
@jessesuen
Copy link
Member

@khhirani can you review this?

@jessesuen jessesuen requested a review from khhirani April 8, 2021 20:02
@huikang
Copy link
Member Author

huikang commented Apr 8, 2021

@jessesuen , are we expecting the following case to be passed?

kind: AnalysisTemplate
apiVersion: argoproj.io/v1alpha1
metadata:
  name: sleep-job
spec:
  args:
  - name: duration
    value: 0s
  - name: exit-code
    value: "0"
  - name: count
    value: "1"
kind: Rollout
metadata:
  name: canary-promote-full
spec:
  replicas: 3
  strategy:
    canary:
      maxUnavailable: 0
      analysis:
        templates:
        - templateName: sleep-job

In this case, the Rollout does not provide any args, but the AnalysisTemplate seems having some default value. Is this a valid usage?

@jessesuen
Copy link
Member

In this case, the Rollout does not provide any args, but the AnalysisTemplate seems having some default value. Is this a valid usage?

Yes, defaulting is a feature of the AnalysisTemplate.

@huikang
Copy link
Member Author

huikang commented Apr 9, 2021

Yes, defaulting is a feature of the AnalysisTemplate.

Thanks. I have updated PR to consider the default values.

Signed-off-by: Hui Kang <hui.kang@salesforce.com>
@sonarcloud
Copy link

sonarcloud bot commented Apr 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@khhirani khhirani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @huikang , your PR looks good to merge

@khhirani khhirani merged commit 74e1f0f into argoproj:master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unresolved argument errors to templates does not surface as validation error
3 participants