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

perf: optimize the modification of rollout to httproute header #137

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

ZhangSetSail
Copy link
Contributor

@ZhangSetSail ZhangSetSail commented Apr 28, 2023

Ⅰ. Describe what this PR does

rollout 对 httproute 的修改存在缺陷,面对 header 的或逻辑并没有支持。

Ⅱ. Does this pull request fix one issue?

是的

Ⅲ. Special notes for reviews

@kruise-bot kruise-bot requested review from FillZpp and veophi April 28, 2023 04:58
@kruise-bot
Copy link

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

@ZhangSetSail
Copy link
Contributor Author

#138

@ZhangSetSail
Copy link
Contributor Author

还存在问题,我再看看

@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2023

Codecov Report

Merging #137 (6066aa2) into master (3578b39) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   44.67%   44.71%   +0.04%     
==========================================
  Files          48       48              
  Lines        5077     5081       +4     
==========================================
+ Hits         2268     2272       +4     
  Misses       2432     2432              
  Partials      377      377              
Flag Coverage Δ
unittests 44.71% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
pkg/trafficrouting/network/gateway/gateway.go 54.76% <100.00%> (+1.48%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ZhangSetSail
Copy link
Contributor Author

测试没问题了,镜像地址:registry.cn-hangzhou.aliyuncs.com/zhangqihang/kruise-rollout:v0428-1

@ZhangSetSail ZhangSetSail force-pushed the rollout_bug branch 3 times, most recently from beae4c2 to 1ed83ba Compare April 28, 2023 08:08
@kruise-bot kruise-bot added size/L and removed size/S labels May 6, 2023
@@ -0,0 +1,36 @@
annotations = {}
Copy link

Choose a reason for hiding this comment

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

0% of developers fix this issue

W111: setting non-standard global variable 'annotations'

❗❗ 5 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 4
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/higress.lua 1
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/higress.lua 5
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/nginx.lua 1
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/nginx.lua 5

Visit the Lift Web Console to find more details in your report.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@@ -0,0 +1,36 @@
annotations = {}
if ( obj.annotations )
Copy link

Choose a reason for hiding this comment

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

0% of developers fix this issue

W113: accessing undefined variable 'obj'

❗❗ 23 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 4
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 13
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 15
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 17
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 19
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 21
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 36
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/higress.lua 3
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/higress.lua 5
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/higress.lua 17

Showing 10 of 23 findings. Visit the Lift Web Console to see all.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

then
annotations = obj.annotations
end
annotations["alb.ingress.kubernetes.io/canary"] = "true"
Copy link

Choose a reason for hiding this comment

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

0% of developers fix this issue

W112: mutating non-standard global variable 'annotations'

❗❗ 33 similar findings have been found in this PR

🔎 Expand here to view all instances of this finding
File Path Line Number
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 7
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 8
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 9
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 10
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 11
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 12
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 15
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 25
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 27
pkg/trafficrouting/network/gateway/lua_configuration/trafficrouting_ingress/aliyun-alb.lua 30

Showing 10 of 33 findings. Visit the Lift Web Console to see all.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@kruise-bot kruise-bot added size/S and removed size/L labels May 6, 2023
@kruise-bot kruise-bot added size/L and removed size/S labels May 6, 2023
@ZhangSetSail ZhangSetSail force-pushed the rollout_bug branch 3 times, most recently from a3906b3 to abe1259 Compare May 6, 2023 06:47
@kruise-bot kruise-bot removed the size/L label May 6, 2023
@zmberg
Copy link
Member

zmberg commented Jun 25, 2023

/lgtm

match.Headers = append(match.Headers, headers...)
canaryRuleMatch := &canaryRule.Matches[j]
for k := range matchs {
match := canaryRuleMatch.DeepCopy()
Copy link
Member

Choose a reason for hiding this comment

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

it seems not necessary to deepcopy here since canaryRule is already a deep clone of rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,我修改一下。

Signed-off-by: zhangsetsail <zqh15131121078@126.com>
@zmberg
Copy link
Member

zmberg commented Jun 27, 2023

/lgtm

@zmberg
Copy link
Member

zmberg commented Jun 28, 2023

/approve

@zmberg
Copy link
Member

zmberg commented Jun 28, 2023

/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

@kruise-bot kruise-bot merged commit 8737f33 into openkruise:master Jun 28, 2023
Kuromesi pushed a commit to Kuromesi/rollouts that referenced this pull request Jul 17, 2023
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.

None yet

5 participants