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

chore: leave the validation of setHeaderRoute to the plugin when plugins is not empty. #2898

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

andyliuliming
Copy link
Contributor

@andyliuliming andyliuliming commented Jul 22, 2023

leave the validation of setHeaderRoute to the plugin when plugins is not empty.
resolving #2899

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.

@andyliuliming andyliuliming changed the title chore: leave the validation of setHeaderRoute to the plugin if plugin… chore: leave the validation of setHeaderRoute to the plugin when plugins is not empty. Jul 22, 2023
@andyliuliming andyliuliming force-pushed the andliu/plugisisoktoo branch 2 times, most recently from 925ed82 to 9f51747 Compare July 22, 2023 11:24
@andyliuliming andyliuliming reopened this Jul 22, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2023

E2E Tests Published Test Results

    4 files      4 suites   3h 48m 21s ⏱️
102 tests   83 ✔️   6 💤 13
428 runs  384 ✔️ 24 💤 20

For more details on these failures, see this check.

Results for commit 358a1e8.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2023

Go Published Test Results

2 047 tests   2 047 ✔️  2m 40s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit 358a1e8.

♻️ This comment has been updated with latest results.

@andyliuliming andyliuliming force-pushed the andliu/plugisisoktoo branch from 9f51747 to 7aac6d4 Compare July 22, 2023 11:29
@sonarcloud
Copy link

sonarcloud bot commented Jul 22, 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
32.6% 32.6% Duplication

@codecov
Copy link

codecov bot commented Jul 22, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (20214b4) 81.73% compared to head (7aac6d4) 81.68%.

❗ Current head 7aac6d4 differs from pull request most recent head 358a1e8. Consider uploading reports for the commit 358a1e8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2898      +/-   ##
==========================================
- Coverage   81.73%   81.68%   -0.05%     
==========================================
  Files         134      133       -1     
  Lines       20406    20309      -97     
==========================================
- Hits        16678    16589      -89     
+ Misses       2869     2862       -7     
+ Partials      859      858       -1     
Files Coverage Δ
pkg/apis/rollouts/validation/validation.go 95.57% <100.00%> (ø)

... and 17 files with indirect coverage changes

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

@zachaller
Copy link
Collaborator

zachaller commented Jul 25, 2023

Can you give an example of why you need this validation removed. Also, argo rollouts supports multiple traffic routers at once I am unsure if it is the right move to turn off validation if say istio and a plugin are used together.

@zachaller zachaller force-pushed the andliu/plugisisoktoo branch from 7aac6d4 to 358a1e8 Compare October 4, 2023 13:52
@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 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
23.3% 23.3% Duplication

@zachaller zachaller added this to the v1.7 milestone Oct 4, 2023
@andyliuliming
Copy link
Contributor Author

andyliuliming commented Dec 5, 2023

Can you give an example of why you need this validation removed. Also, argo rollouts supports multiple traffic routers at once I am unsure if it is the right move to turn off validation if say istio and a plugin are used together.

@zachaller we are trying to use the max traffic weight > 100.

so 2 ways for us:

  1. use the set traffic header to do the header regex
  2. add support for the max weight > 100 as I was trying to do it in this PR:
    https://github.com/argoproj/argo-rollouts/pull/2900/files#diff-d4a6b587ffdebe4f22c2cc8b65d601c03c9eb897a06f36c1f633778db3fbe58e
    but that PR is more complex than I expected, since looks like the max weight is not in the spec, so some breaking change may needed, I'm still going through the code to try to find a way to not add breaking change

this PR is to try the #1 way.

@andyliuliming
Copy link
Contributor Author

andyliuliming commented Dec 5, 2023

@zachaller maybe add one field
MaxTrafficWeight *int32 json:"maxTrafficWeight,omitempty" protobuf:"varint,10,opt,name=maxTrafficWeight" into RolloutTrafficRouting struct is accepatable?

drafting PR:
#3215

I will try to do some sanity check on the usage. and add ut later.

@zachaller zachaller merged commit 8d20ab7 into argoproj:master Dec 8, 2023
20 checks passed
ashutosh16 pushed a commit to ashutosh16/argo-rollouts that referenced this pull request Dec 8, 2023
…ins is not empty. (argoproj#2898)

chore: leave the validation of setHeaderRoute to the plugin if plugins not empty.

Signed-off-by: Liming Liu <[email protected]>
Co-authored-by: Ubuntu <andliu@devbox.5xpt1tfa54mehhcinhsnwwrpve.ix.internal.cloudapp.net>
Signed-off-by: ashutosh16 <[email protected]>
emily-blixt-ck pushed a commit to emily-blixt-ck/argo-rollouts that referenced this pull request Dec 8, 2023
…ins is not empty. (argoproj#2898)

chore: leave the validation of setHeaderRoute to the plugin if plugins not empty.

Signed-off-by: Liming Liu <[email protected]>
Co-authored-by: Ubuntu <andliu@devbox.5xpt1tfa54mehhcinhsnwwrpve.ix.internal.cloudapp.net>
denis-codefresh pushed a commit to codefresh-io/argo-rollouts that referenced this pull request Jul 1, 2024
…ins is not empty. (argoproj#2898)

chore: leave the validation of setHeaderRoute to the plugin if plugins not empty.

Signed-off-by: Liming Liu <[email protected]>
Co-authored-by: Ubuntu <andliu@devbox.5xpt1tfa54mehhcinhsnwwrpve.ix.internal.cloudapp.net>
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