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(controller): weighted experiment validation should allow delegating to trafficRouter plugins #3909

Conversation

bezmax
Copy link
Contributor

@bezmax bezmax commented Oct 22, 2024

Even though weighted experiment calls are correctly delegated to Traffic Router plugins, the validation is currently not allowing to register a TrafficRouting plugin when weighted experiments are present.

It seems that this validation was added before plugin architecture supported additionalDestinations, so it made sense then. Ideally a plugin would have a way to announce it's capabilities, but given right now API does not have anything suitable for that - this PR provides a simple way to unblock using plugins that support weighted experiments.

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.

@bezmax bezmax force-pushed the maksimb/allow-plugins-for-weighted-experiments branch from aa87e33 to 8d7e677 Compare October 22, 2024 19:25
…ng to trafficRouter plugins

Signed-off-by: Maksim Bezsaznyj <bezmax@gmail.com>
@bezmax bezmax force-pushed the maksimb/allow-plugins-for-weighted-experiments branch from 8d7e677 to 0450cd4 Compare October 22, 2024 19:32
Copy link

sonarcloud bot commented Oct 22, 2024

Copy link
Contributor

Published E2E Test Results

  4 files    4 suites   3h 15m 28s ⏱️
113 tests 103 ✅  7 💤 3 ❌
456 runs  424 ✅ 28 💤 4 ❌

For more details on these failures, see this check.

Results for commit 0450cd4.

Copy link
Contributor

Published Unit Test Results

2 279 tests   2 279 ✅  2m 59s ⏱️
  128 suites      0 💤
    1 files        0 ❌

Results for commit 0450cd4.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.70%. Comparing base (1264f8d) to head (0450cd4).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3909      +/-   ##
==========================================
+ Coverage   82.69%   82.70%   +0.01%     
==========================================
  Files         163      163              
  Lines       22886    22886              
==========================================
+ Hits        18925    18928       +3     
+ Misses       3087     3085       -2     
+ Partials      874      873       -1     

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

@zachaller zachaller merged commit 8503baa into argoproj:master Oct 29, 2024
26 checks passed
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