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): Make AnalysisRun end when only Dry-Run metrics are defined. Fixes: #2151 #2230

Merged
merged 2 commits into from
Sep 22, 2022

Conversation

agrawroh
Copy link
Member

@agrawroh agrawroh commented Sep 5, 2022

Description

This PR fixes the issue defined in #2151 where AnalysisRun doesn't transition to Successful from Running when only Dry-Run metrics are defined and being evaluated. As per the current logic, we don't set the decision while evaluating the Dry-Run metrics but the proper fix is to set it to Successful at least if nothing is set.

Checklist

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@agrawroh agrawroh added the analysis Related to Analysis CRD label Sep 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2022

Go Published Test Results

1 769 tests   1 769 ✔️  2m 34s ⏱️
   101 suites         0 💤
       1 files           0

Results for commit 49df77f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2022

E2E Tests Published Test Results

  1 files    1 suites   46m 21s ⏱️
89 tests 84 ✔️ 3 💤 2
91 runs  86 ✔️ 3 💤 2

For more details on these failures, see this check.

Results for commit 49df77f.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Base: 82.35% // Head: 82.36% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (49df77f) compared to base (e646371).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2230   +/-   ##
=======================================
  Coverage   82.35%   82.36%           
=======================================
  Files         121      121           
  Lines       18434    18438    +4     
=======================================
+ Hits        15182    15186    +4     
  Misses       2466     2466           
  Partials      786      786           
Impacted Files Coverage Δ
analysis/analysis.go 85.53% <100.00%> (+0.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

analysis/analysis_test.go Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Sep 16, 2022

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 1 Code Smell

No Coverage information No Coverage information
9.2% 9.2% Duplication

@chrisplim
Copy link

@agrawroh thanks for the fix and @harikrongali thanks for the review!

@jessesuen jessesuen merged commit ae56d91 into argoproj:master Sep 22, 2022
daixijun pushed a commit to daixijun/argo-rollouts that referenced this pull request Sep 27, 2022
leoluz pushed a commit that referenced this pull request Oct 6, 2022
jenciso pushed a commit to jenciso/argo-rollouts that referenced this pull request Oct 25, 2022
jandersen-plaid pushed a commit to jandersen-plaid/argo-rollouts that referenced this pull request Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Related to Analysis CRD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants