Skip to content

Conversation

wilkermichael
Copy link
Contributor

@wilkermichael wilkermichael commented Jan 30, 2024

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.

Copy link
Contributor

github-actions bot commented Jan 30, 2024

Go Published Test Results

2 124 tests   2 124 ✅  2m 50s ⏱️
  118 suites      0 💤
    1 files        0 ❌

Results for commit 74cf023.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 69 lines in your changes are missing coverage. Please review.

Comparison is base (8405f2e) 81.83% compared to head (74cf023) 82.86%.
Report is 9 commits behind head on master.

Files Patch % Lines
controller/controller.go 21.68% 61 Missing and 4 partials ⚠️
utils/record/record.go 50.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3339      +/-   ##
==========================================
+ Coverage   81.83%   82.86%   +1.02%     
==========================================
  Files         135      135              
  Lines       20688    16845    -3843     
==========================================
- Hits        16931    13959    -2972     
+ Misses       2883     2012     -871     
  Partials      874      874              

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

Copy link
Contributor

github-actions bot commented Jan 30, 2024

E2E Tests Published Test Results

  4 files    4 suites   3h 31m 5s ⏱️
106 tests  96 ✅  6 💤 4 ❌
428 runs  400 ✅ 24 💤 4 ❌

For more details on these failures, see this check.

Results for commit 74cf023.

♻️ This comment has been updated with latest results.

@zachaller zachaller changed the title Add exception to requireCanaryStableServices to disable validation when using the hashicorp/consul plugin chore: Add exception to requireCanaryStableServices to disable validation when using the hashicorp/consul plugin Jan 30, 2024
@wilkermichael wilkermichael force-pushed the master branch 2 times, most recently from 28a3cbe to d0287fd Compare February 1, 2024 21:44
@wilkermichael wilkermichael marked this pull request as ready for review February 5, 2024 19:11
@wilkermichael wilkermichael force-pushed the master branch 5 times, most recently from 25eee48 to 647ca89 Compare February 6, 2024 16:08
@david-yu
Copy link

david-yu commented Feb 9, 2024

Hi @zachaller friendly nudge, if you're able to review this PR for Consul Service Mesh.

@zachaller zachaller added this to the v1.7 milestone Feb 9, 2024
@@ -129,6 +129,80 @@ func TestValidateRolloutStrategyBlueGreen(t *testing.T) {
assert.Equal(t, ScaleDownLimitLargerThanRevisionLimit, allErrs[1].Detail)
}

func TestValidateRolloutStrategyCanaryMissingServiceNames(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a few test that use two traffic routers one where both are required to have canary service and one where it one requires it and one does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added some test cases with multiple routers where one requires the canary services and one or more do not to ensure they error as expected.

Michael Wilkerson and others added 3 commits February 12, 2024 08:29
…when using the `hashicorp/consul` plugin

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>
Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>
- added test cases for exceptions to requiring checks for Canary/Stable services
- added test cases for verifying errors when Canary/Stable services are missing

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>
Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>
- added test cases for exceptions to requiring checks for Canary/Stable services
- added test cases for verifying errors when Canary/Stable services are missing

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@zachaller zachaller enabled auto-merge (squash) February 12, 2024 17:22
@zachaller zachaller disabled auto-merge February 12, 2024 17:23
@thisisnotashwin
Copy link
Contributor

Thanks so much for helping with this PR @zachaller

@zachaller zachaller merged commit 724d53d into argoproj:master Feb 12, 2024
meeech pushed a commit to CircleCI-Public/argo-rollouts that referenced this pull request Feb 10, 2025
…dation when using the `hashicorp/consul` plugin (argoproj#3339)

* add exception to `requireCanaryStableServices` to disable validation when using the `hashicorp/consul` plugin

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>

* added HashiCorp to USERS.md

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>

* modified requireCanaryStableServices to reverse logic
- added test cases for exceptions to requiring checks for Canary/Stable services
- added test cases for verifying errors when Canary/Stable services are missing

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>

* addressed comments and added more test cases

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>

* modified requireCanaryStableServices to reverse logic
- added test cases for exceptions to requiring checks for Canary/Stable services
- added test cases for verifying errors when Canary/Stable services are missing

Co-authored-by: Ashwin Venkatesh <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>

---------

Signed-off-by: Michael Wilkerson <[email protected]>
Co-authored-by: Ashwin Venkatesh <[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.

4 participants