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

Allow Not generating Canary Service && Fixed a bug caused by NOT considering case-insensitivity. #200

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

myname4423
Copy link
Contributor

@myname4423 myname4423 commented Jan 18, 2024

Ⅰ. Describe what this PR does

  1. Allow Not generating Canary Service when using trafficRoutings.customNetworkRefs
  2. Fixed a bug caused by NOT considering case-insensitivity.

Ⅱ. Does this pull request fix one issue?

NO

Ⅲ. Special notes for reviews

For 1:
when working with trafficRoutings.customNetworkRefs (such as VirtualService and DestinationRule for Istio), rollout will always generate canary Service based on the provided Service, which may be undesirable in the scenario of using DestinationRule subsets.

For 2:
In the code within api/v1alpha1/conversion.go, when converting objects from v1alpha1 to v1beta1 (aka. the HUB version), a direct string comparison is made. This leads to an issue where v1alpha1 objects (Rollout and batchRelease) are only converted correctly to the v1beta1 object if the RolloutStyleAnnotation annotation is exactly "Partition" (with an uppercase initial letter). In other cases, where RolloutStyleAnnotation is "partition", "canary", or "Canary", the conversion defaults to the canary.

@kruise-bot
Copy link

Welcome @myname4423! It looks like this is your first PR to openkruise/rollouts 🎉

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 43.42%. Comparing base (678d4d2) to head (58d5309).

Files Patch % Lines
pkg/trafficrouting/manager.go 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #200   +/-   ##
=======================================
  Coverage   43.41%   43.42%           
=======================================
  Files          52       52           
  Lines        5675     5677    +2     
=======================================
+ Hits         2464     2465    +1     
- Misses       2786     2787    +1     
  Partials      425      425           
Flag Coverage Δ
unittests 43.42% <88.23%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

Signed-off-by: yunbo <yunbo10124scut@gmail.com>
@kruise-bot
Copy link

New changes are detected. LGTM label has been removed.

@kruise-bot kruise-bot added size/M and removed size/S labels Mar 6, 2024
@myname4423 myname4423 closed this Mar 6, 2024
@myname4423 myname4423 reopened this Mar 6, 2024
@myname4423 myname4423 changed the title Fixed a bug caused by NOT considering case-insensitivity. Allow Not generate Canary Service && Fixed a bug caused by NOT considering case-insensitivity. Mar 7, 2024
@myname4423 myname4423 changed the title Allow Not generate Canary Service && Fixed a bug caused by NOT considering case-insensitivity. Allow Not generating Canary Service && Fixed a bug caused by NOT considering case-insensitivity. Mar 7, 2024
amend1: update crd yaml

amend2: add DisableGenerateCanaryService for v1alpha1

Signed-off-by: yunbo <yunbo10124scut@gmail.com>
@zmberg
Copy link
Member

zmberg commented Mar 12, 2024

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zmberg zmberg merged commit 0ff23f6 into openkruise:master Mar 12, 2024
20 of 21 checks passed
myname4423 pushed a commit to myname4423/rollouts that referenced this pull request Mar 13, 2024
Signed-off-by: yunbo <yunbo10124scut@gmail.com>
myname4423 pushed a commit to myname4423/rollouts that referenced this pull request Mar 13, 2024
Signed-off-by: yunbo <yunbo10124scut@gmail.com>
lujiajing1126 pushed a commit to lujiajing1126/rollouts that referenced this pull request Mar 15, 2024
…idering case-insensitivity. (openkruise#200)

* Fixed a bug caused by NOT considering case-insensitivity.

Signed-off-by: yunbo <yunbo10124scut@gmail.com>

* add DisableGenerateCanaryService for CanaryStrategy

amend1: update crd yaml

amend2: add DisableGenerateCanaryService for v1alpha1

Signed-off-by: yunbo <yunbo10124scut@gmail.com>

---------

Signed-off-by: yunbo <yunbo10124scut@gmail.com>
Co-authored-by: yunbo <yunbo10124scut@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
kruise-bot pushed a commit that referenced this pull request Apr 2, 2024
Signed-off-by: yunbo <yunbo10124scut@gmail.com>
Co-authored-by: yunbo <yunbo10124scut@gmail.com>
kruise-bot pushed a commit that referenced this pull request Jun 12, 2024
* support queryparams for gateway api

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* support mse

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* support path and queryParams

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* fix lint

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* fix testcase

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* fix manifests

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* do not provide default value for path

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* Allow Not generating Canary Service && Fixed a bug caused by NOT considering case-insensitivity. (#200)

* Fixed a bug caused by NOT considering case-insensitivity.

Signed-off-by: yunbo <yunbo10124scut@gmail.com>

* add DisableGenerateCanaryService for CanaryStrategy

amend1: update crd yaml

amend2: add DisableGenerateCanaryService for v1alpha1

Signed-off-by: yunbo <yunbo10124scut@gmail.com>

---------

Signed-off-by: yunbo <yunbo10124scut@gmail.com>
Co-authored-by: yunbo <yunbo10124scut@gmail.com>
Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* revert test images

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* polish comments

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* add gateway api tests

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* fix MSE cases

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* update golang lint ci

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* regenerate manifests

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* remove generic usage

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* update istio lua script

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* update v1alpha1 in e2e to v1beta1

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* fix cases

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* refactor istio case to include queryParams and path

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* fix cloneset issue

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* fix typo

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

* revert images

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>

---------

Signed-off-by: Megrez Lu <lujiajing1126@gmail.com>
Signed-off-by: yunbo <yunbo10124scut@gmail.com>
Co-authored-by: myname4423 <57184070+myname4423@users.noreply.github.com>
Co-authored-by: yunbo <yunbo10124scut@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants