Skip to content

[FEATURE]: 添加 mustConnect #202

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jan 23, 2022
Merged

[FEATURE]: 添加 mustConnect #202

merged 6 commits into from
Jan 23, 2022

Conversation

yellowmarlboro
Copy link
Contributor

我只能想到这么写，如果有更好的实现方式，请提出。第一次提pr，不太会写，谢谢。

@mergify
Copy link
Contributor

mergify bot commented Dec 22, 2021

感谢您提出Pull Request，我会尽快Review。我会在1-2日内进行查看或者回复，如果遇到节假日可能会处理较慢，敬请谅解。

zouyx
zouyx reviewed Dec 27, 2021
View reviewed changes
Copy link
Member

@zouyx zouyx left a comment

Choose a reason for hiding this comment

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

好像 UT 没过- -

@yellowmarlboro
Copy link
Contributor Author

我看了好像是拉依赖的时候报错？还没看到UT的日志，这种情况我可以做什么吗

@coveralls
Copy link

coveralls commented Dec 31, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1611383904

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
  • 96 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-1.3%) to 74.748%
Changes Missing Coverage Covered Lines Changed/Added Lines %
client.go 3 5 60.0%
Files with Coverage Reduction New Missed Lines %
component/remote/abs.go 3 70.59%
component/remote/sync.go 17 65.67%
component/remote/async.go 21 77.5%
env/config/config.go 24 42.11%
client.go 31 74.59%
Totals Coverage Status
Change from base Build 1573832883: -1.3%
Covered Lines: 1187
Relevant Lines: 1588
💛 - Coveralls

@zouyx zouyx self-assigned this Jan 6, 2022
zouyx
zouyx reviewed Jan 6, 2022
View reviewed changes
Copy link
Member

@zouyx zouyx left a comment •
edited
Loading

Choose a reason for hiding this comment

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

十分感谢你提交的 PR。我大概看了一下，我有些思考，不知道是否能帮助到你。
先分析一下这个需求：

  • 是否能通过不加配置实现？因为我觉得用户觉得比较麻烦。
  • panic 是否是可以交由用户实现？站在用户的角度，一调用就 panic。用户估计不会使用。其次，站在我的角度不想因为 panic 导致 testcase 需要跳过，testcase 是保证用户质量的非常重要的手段。

最后我有些�建议，但不知道是否可以实施：

  • 增加一个入口方法（ 和我想的一样
  • 在启动的时候，因为会先同步一次。

    agollo/client.go

    Line 124 in 9b72755

    configs := syncApolloConfig.Sync(c.getAppConfig)
  • 是否重构启动的方法，在其中做文章？

@yellowmarlboro
Copy link
Contributor Author

我又重新修改了一下，去掉了必须从远程获取的概念，改为第一次启动获取不到配置就返回错误的方式。

zouyx
zouyx reviewed Jan 10, 2022
View reviewed changes
zouyx
zouyx reviewed Jan 21, 2022
View reviewed changes
@zouyx zouyx changed the base branch from master to develop January 22, 2022 16:48
@zouyx zouyx self-requested a review January 23, 2022 05:26
@mergify mergify bot merged commit 9a239db into apolloconfig:develop Jan 23, 2022
@zouyx zouyx linked an issue Jan 23, 2022 that may be closed by this pull request
[FEATURE] 有没有mustConnect类似的实现？ #201
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zouyx zouyx zouyx approved these changes

Assignees

@zouyx zouyx

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] 有没有mustConnect类似的实现？
4 participants
@yellowmarlboro @coveralls @zouyx @siwenzhe