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: Analysis argument validation #1412

Merged
merged 16 commits into from
Aug 25, 2021
Merged

Conversation

khhirani
Copy link
Contributor

Fixes #1358

@khhirani
Copy link
Contributor Author

@harikrongali Please review when you get a chance

@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #1412 (57eae70) into master (f9d2940) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1412      +/-   ##
==========================================
- Coverage   81.54%   81.46%   -0.08%     
==========================================
  Files         108      108              
  Lines       10104    10124      +20     
==========================================
+ Hits         8239     8248       +9     
- Misses       1304     1312       +8     
- Partials      561      564       +3     
Impacted Files Coverage Δ
analysis/analysis.go 83.65% <100.00%> (-2.23%) ⬇️

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 f9d2940...57eae70. Read the comment docs.

Signed-off-by: khhirani <[email protected]>
@jessesuen
Copy link
Member

Does this need to be cherry picked to v1.0.x?

@harikrongali
Copy link
Contributor

harikrongali commented Aug 24, 2021

args for count is not working.https://github.com/argoproj/argo-rollouts/blob/986d4b045ddd2d604bdf8e372dedb40914b37717/pkg/apis/rollouts/v1alpha1/openapi_generated.go

--- ignore the issue as the validation coming from API and for now there is workaround where default value can be replaced from rollout.

Signed-off-by: hari rongali <[email protected]>
Signed-off-by: hari rongali <[email protected]>
@harikrongali
Copy link
Contributor

added e2e tests in khhirani#111 . please verify the E2e tests. Other than that changes looks good

harikrongali and others added 3 commits August 24, 2021 17:42
Signed-off-by: hari rongali <[email protected]>
Signed-off-by: hari rongali <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
6.9% 6.9% Duplication

@harikrongali
Copy link
Contributor

approve. LGTM

@khhirani khhirani requested a review from alexmt August 25, 2021 18:58
Copy link
Contributor

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM.

Tested manually as well with the example provided in the issue - no validation errors

@alexmt alexmt merged commit a601a0c into argoproj:master Aug 25, 2021
alexmt pushed a commit that referenced this pull request Aug 26, 2021
* fix: Analysis argument validation

Signed-off-by: khhirani <[email protected]>
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.

Analysis with arguments not working
4 participants