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: configuration API returns wrong store_name #641

Merged
merged 23 commits into from
Jun 21, 2022

Conversation

MichaelDeSteven
Copy link
Contributor

What this PR does:

Which issue(s) this PR fixes:

Fixes #613

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@mosn-community-bot mosn-community-bot bot added bug Something isn't working cla:yes size/S labels Jun 11, 2022
@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #641 (1c82c02) into main (cef86c7) will increase coverage by 0.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   60.92%   60.93%   +0.01%     
==========================================
  Files         120      120              
  Lines        6377     6384       +7     
==========================================
+ Hits         3885     3890       +5     
- Misses       2120     2121       +1     
- Partials      372      373       +1     
Impacted Files Coverage Δ
components/configstores/apollo/repository.go 12.12% <ø> (ø)
components/configstores/apollo/configstore.go 74.78% <60.00%> (-0.22%) ⬇️
components/configstores/apollo/change_listener.go 82.60% <100.00%> (ø)
components/configstores/etcdv3/etcdv3.go 81.34% <100.00%> (+0.14%) ⬆️
pkg/runtime/runtime.go 60.48% <100.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb34d2a...1c82c02. Read the comment docs.

@seeflood
Copy link
Member

Thanks for your help!
@wenxuwan Could u help review this PR?

wenxuwan
wenxuwan previously approved these changes Jun 14, 2022
Copy link
Member

@wenxuwan wenxuwan left a comment

Choose a reason for hiding this comment

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

LGTM

@wenxuwan
Copy link
Member

image

在这里做一下替换吧,把config.StoreName替换成name,然后在Init的时候赋值

@MichaelDeSteven
Copy link
Contributor Author

@wenxuwan 替换好了

@wenxuwan
Copy link
Member

@wenxuwan 替换好了

上图里面Init函数的入参config里面的storeName现在是个空值,你需要在我贴的图的代码处给他复制成name,然后sub的时候就不需要再重新赋值了。

@wenxuwan
Copy link
Member

apollo的实现也有同样的问题,可以一起修复掉

@MichaelDeSteven
Copy link
Contributor Author

apollo的实现也有同样的问题,可以一起修复掉

使用apollo的默认配置报错了。能改回原来的配置吗,方便测试。

@wenxuwan
Copy link
Member

wenxuwan commented Jun 17, 2022

apollo的实现也有同样的问题,可以一起修复掉

使用apollo的默认配置报错了。能改回原来的配置吗,方便测试。

什么意思?可以本地改一下配置文件,进行测试。

@MichaelDeSteven
Copy link
Contributor Author

MichaelDeSteven commented Jun 19, 2022

apollo的实现也有同样的问题,可以一起修复掉

使用apollo的默认配置报错了。能改回原来的配置吗,方便测试。

什么意思?可以本地改一下配置文件,进行测试。

我直接运行Layotto server端,报401
image
image
查了下apollo文档结果如下图,但是配置文件的token应该是没问题的,应该怎么改配置文件,server才能跑起来?
image

@wenxuwan
Copy link
Member

apollo的实现也有同样的问题,可以一起修复掉

使用apollo的默认配置报错了。能改回原来的配置吗,方便测试。

什么意思?可以本地改一下配置文件,进行测试。

我直接运行Layotto server端,报401 image image 查了下apollo文档结果如下图,但是配置文件的token应该是没问题的,应该怎么改配置文件,server才能跑起来? image

这个错误不影响server启动的,你可以按照文档跑client demo,不会影响能力。

@MichaelDeSteven
Copy link
Contributor Author

@wenxuwan apollo部分已修复,有空麻烦review一下

@wenxuwan
Copy link
Member

wenxuwan commented Jun 20, 2022

please fix Layotto Env Pipeline 🌊 / 🍀 DeadLink Validation (pull_request)

@MichaelDeSteven
Copy link
Contributor Author

please fix Layotto Env Pipeline 🌊 / 🍀 DeadLink Validation (pull_request)

这个deadLink该怎么修复呢,有时候检查能过,有时候会失败

@seeflood
Copy link
Member

这个deadLink该怎么修复呢,有时候检查能过,有时候会失败

@MichaelDeSteven 有的url 不太稳定,可能在跑ci的时候 github 容器调那个url调不通、就报错了;这种时候重试下(重新运行下 workflow )就行啦

Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

lgtm

@seeflood seeflood merged commit 66e64e9 into mosn:main Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla:yes size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: configuration API returns wrong app_id and store_name
4 participants