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: 🐛 [frontend-gray] 修复 请求非首页资源时候,路由配置 #1353

Merged
merged 5 commits into from
Oct 8, 2024

Conversation

heimanba
Copy link
Contributor

[bugfix] 修复 请求非首页资源时候,路由配置

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.25%. Comparing base (ef31e09) to head (9b4d1a2).
Report is 138 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1353      +/-   ##
==========================================
+ Coverage   35.91%   44.25%   +8.34%     
==========================================
  Files          69       76       +7     
  Lines       11576     9895    -1681     
==========================================
+ Hits         4157     4379     +222     
+ Misses       7104     5182    -1922     
- Partials      315      334      +19     

see 91 files with indirect coverage changes

deployment = util.FilterGrayWeight(&grayConfig, preVersion, preUniqueClientId, uniqueClientId)
} else {
deployment = util.FilterGrayRule(&grayConfig, grayKeyValue)
}
log.Infof("index deployment: %v, path: %v, backend: %v, xPreHigressVersion: %s,%s", deployment, path, deployment.BackendVersion, preVersion, preUniqueClientId)
} else {
deployment = util.GetVersion(grayConfig, deployment, preVersion, isPageRequest)
grayDeployment := deployment
if deployment.Version == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个地方 deployment.Version 可能不是空吗?

Copy link
Collaborator

Choose a reason for hiding this comment

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

我捋一下这段的逻辑哦。

  • 如果是页面请求:
    • 如果配了权重,就按权重选择目标版本
    • 如果没配权重,就按规则选择目标版本
    • 这里不考虑当前请求中是否包含灰度 Cookie
  • 如果不是页面请求:
    • 不看权重,只按规则选择目标版本
    • 如果 Cookie 中包含当前灰度版本信息,则优先选择 Cookie 中声明的版本

请问我理解的对吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个地方 deployment.Version 可能不是空吗?

可能的 ,因为有可能不经过首页html。直接访问 .css这样的文件,灰度规则会失效

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我捋一下这段的逻辑哦。

  • 如果是页面请求:

    • 如果配了权重,就按权重选择目标版本
    • 如果没配权重,就按规则选择目标版本
    • 这里不考虑当前请求中是否包含灰度 Cookie
  • 如果不是页面请求:

    • 不看权重,只按规则选择目标版本
    • 如果 Cookie 中包含当前灰度版本信息,则优先选择 Cookie 中声明的版本

请问我理解的对吗?

是的,有Cookie的话,就认为是 页面请求种下的Cookie,这样可以做请求黏贴。

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个地方 deployment.Version 可能不是空吗?

可能的 ,因为有可能不经过首页html。直接访问 .css这样的文件,灰度规则会失效

但根据下图,deployment 对象创建之后,直接就会进行 version 是否为空的判定。这里应该不会不为空吧?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个地方 deployment.Version 可能不是空吗?

可能的 ,因为有可能不经过首页html。直接访问 .css这样的文件,灰度规则会失效

但根据下图,deployment 对象创建之后,直接就会进行 version 是否为空的判定。这里应该不会不为空吧?

image

是的,这里不需要判空了,已经修复。非常感谢

Copy link
Collaborator

@CH3CHO CH3CHO left a comment

Choose a reason for hiding this comment

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

LGTM

@CH3CHO CH3CHO merged commit 4d0d8a7 into alibaba:main Oct 8, 2024
12 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.

3 participants