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: append weighted destination only when weight is mentioned #2734

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

divyansh375
Copy link
Contributor

@divyansh375 divyansh375 commented Apr 19, 2023

As of now, in canary deployment with weighted experiment, the rollout throws nil pointer exception, when in an experiment step, one template contains weight and the other doesn't. For example:

Screenshot 2023-04-19 at 5 03 25 PM

Hence handling a nil pointer check and also a UT verification for port name .

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 with a list of types and scopes found here, (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.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2023

Go Published Test Results

2 097 tests   2 097 ✅  2m 49s ⏱️
  118 suites      0 💤
    1 files        0 ❌

Results for commit e09f049.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (723f7a9) 81.79% compared to head (e09f049) 81.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2734      +/-   ##
==========================================
+ Coverage   81.79%   81.87%   +0.08%     
==========================================
  Files         135      135              
  Lines       20649    20650       +1     
==========================================
+ Hits        16889    16908      +19     
+ Misses       2885     2869      -16     
+ Partials      875      873       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2023

E2E Tests Published Test Results

  4 files    4 suites   3h 26m 55s ⏱️
106 tests  95 ✅  6 💤 5 ❌
430 runs  401 ✅ 24 💤 5 ❌

For more details on these failures, see this check.

Results for commit e09f049.

♻️ This comment has been updated with latest results.

@zachaller
Copy link
Collaborator

Can we also add a unit test for the nil pointer check?

@divyansh375 divyansh375 force-pushed the fix-weighted-experiment-case branch from 4b7f279 to 90e544c Compare April 20, 2023 16:06
@divyansh375
Copy link
Contributor Author

Hi @zachaller thanks for reviewing, let me add that as well

@divyansh375 divyansh375 force-pushed the fix-weighted-experiment-case branch 5 times, most recently from 31816e6 to 81bf063 Compare May 7, 2023 11:18
@divyansh375
Copy link
Contributor Author

divyansh375 commented May 7, 2023

Hi @zachaller please could you review this? apologies for taking so long to address your comment, I have made few changes in the existing UT itself because the existing UT "TestRolloutWithExperimentStep" was not covering the desired code.. It was logging error but somehow the UT was passing.

@divyansh375 divyansh375 force-pushed the fix-weighted-experiment-case branch from 81bf063 to 9acfde0 Compare May 9, 2023 16:50
@divyansh375
Copy link
Contributor Author

@zachaller Hi please could you update on this?

@divyansh375
Copy link
Contributor Author

@zachaller please let me know if any further change is required before we can merge this

@divyansh375
Copy link
Contributor Author

@leoluz please could you review?

@divyansh375
Copy link
Contributor Author

@zachaller please can you review

@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2023

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 0 Code Smells

No Coverage information No Coverage information
3.7% 3.7% Duplication

@divyansh375
Copy link
Contributor Author

@zachaller please can you review

@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2023

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 0 Code Smells

No Coverage information No Coverage information
15.9% 15.9% Duplication

@zachaller zachaller added this to the v1.7 milestone Nov 22, 2023
@zachaller zachaller force-pushed the fix-weighted-experiment-case branch from 45222e0 to 299c344 Compare January 2, 2024 16:17
@zachaller zachaller force-pushed the fix-weighted-experiment-case branch from 299c344 to e09f049 Compare January 26, 2024 20:14
Copy link

sonarcloud bot commented Jan 26, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
8.5% Duplication on New Code

See analysis details on SonarCloud

@zachaller zachaller merged commit dced99b into argoproj:master Feb 13, 2024
23 checks passed
@chetan-rns chetan-rns mentioned this pull request Aug 30, 2024
2 tasks
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.

2 participants