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

feature:expands grayscale publishing capabilities to support dimensio… #4013

Merged
merged 10 commits into from
Oct 9, 2021

Conversation

zcy1010
Copy link
Contributor

@zcy1010 zcy1010 commented Oct 5, 2021

What's the purpose of this PR

Add new feature:

  • Apollo expands grayscale publishing capabilities to support dimensions other than IP

Which issue(s) this PR fixes:

Fixes #2932

Brief changelog

  • The related interfaces for server-side modification and acquisition of gray rules are mainly (assembleQueryConfigUr in RemoteConfigRepository.java, loadConfig in ConfigService, findReleaseIdFromGrayReleaseRule in GrayReleaseRulesHolder)
  • Modified the interface of the front-end configuration grayscale rules
  • Modified the document for configuring grayscale rules

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@github-actions
Copy link

github-actions bot commented Oct 5, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@zcy1010
Copy link
Contributor Author

zcy1010 commented Oct 5, 2021

I have read the CLA Document and I hereby sign the CLA

@Anilople
Copy link
Contributor

Anilople commented Oct 6, 2021

Is app.label a json?

i.e

{
  "ip": "10.240.50.100",
  "appId": "my-app-id"
}

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #4013 (9fcc535) into master (8484178) will increase coverage by 0.08%.
The diff coverage is 70.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4013      +/-   ##
============================================
+ Coverage     51.46%   51.54%   +0.08%     
- Complexity     2524     2533       +9     
============================================
  Files           484      484              
  Lines         14798    14839      +41     
  Branches       1532     1537       +5     
============================================
+ Hits           7616     7649      +33     
- Misses         6652     6658       +6     
- Partials        530      532       +2     
Impacted Files Coverage Δ
...ring/boot/ApolloApplicationContextInitializer.java 94.33% <ø> (ø)
...work/apollo/common/dto/GrayReleaseRuleItemDTO.java 0.00% <0.00%> (ø)
...ramework/apollo/core/ApolloClientSystemConsts.java 0.00% <ø> (ø)
...rk/foundation/internals/provider/NullProvider.java 0.00% <0.00%> (ø)
...apollo/openapi/dto/OpenGrayReleaseRuleItemDTO.java 0.00% <0.00%> (ø)
...ramework/apollo/openapi/util/OpenApiBeanUtils.java 8.82% <0.00%> (-0.14%) ⬇️
...ework/apollo/internals/RemoteConfigRepository.java 88.34% <33.33%> (-1.04%) ⬇️
...internals/provider/DefaultApplicationProvider.java 49.16% <35.00%> (-2.84%) ⬇️
...configservice/controller/ConfigFileController.java 83.47% <66.66%> (ø)
...ctrip/framework/apollo/portal/environment/Env.java 92.75% <92.64%> (+23.90%) ⬆️
... and 7 more

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 3d0925f...9fcc535. Read the comment docs.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

This feature looks great! Please find some comments below.

BTW, there are some bugs regarding the grayscale rule setup for public namespaces. I fixed 2 of them in the comments and there is one bug left to you to fix.

  1. first set up the grayscale rule to apply to all instances and save
    image

  2. then edit the rules to change it to apply to some instances and add label 'some-label'
    image

image

  1. edit the rule again, and it shows as apply to all instances
    image

@zcy1010
Copy link
Contributor Author

zcy1010 commented Oct 6, 2021

Is app.label a json?

i.e

{
  "ip": "10.240.50.100",
  "appId": "my-app-id"
}

Yes, the storage format of app.label is the same as app.ip

@nobodyiam
Copy link
Member

BTW, please also update the CHANGES.md

@zcy1010
Copy link
Contributor Author

zcy1010 commented Oct 6, 2021

BTW, please also update the CHANGES.md

Thank you for your review, I have modified according to your comments.

@nobodyiam
Copy link
Member

BTW, please also update the CHANGES.md

Thank you for your review, I have modified according to your comments.

Looks great now!
One last thing about the bug mentioned, I think the correct behavior when user switches from apply to all instance to apply to some instance is to reset the selections from * to empty, just like when user creates a new rule.

Current behavior
image

Expected behavior
image

@Anilople
Copy link
Contributor

Anilople commented Oct 7, 2021

How about use config

apollo.labels.ip=xxx
apollo.labels.zone=beijing
apollo.labels.bu=pay

to represent

{
  "ip": "xxx",
  "zone": "beijing",
  "bu": "pay"
}

i.e User can write multiple lines instead of write all in one line.

One line is represent a key value pair.


In spring, the properties class is

@ConfigurationProperties(prefix = "apollo")
public class ApolloLabelsProperties {
  private Map<String, String> labels;
}

labels will load on runtime.

We can manually load it without spring.

@zcy1010
Copy link
Contributor Author

zcy1010 commented Oct 7, 2021

How about use config

apollo.labels.ip=xxx
apollo.labels.zone=beijing
apollo.labels.bu=pay

to represent

{
  "ip": "xxx",
  "zone": "beijing",
  "bu": "pay"
}

i.e User can write multiple lines instead of write all in one line.

One line is represent a key value pair.

In spring, the properties class is

@ConfigurationProperties(prefix = "apollo")
public class ApolloLabelsProperties {
  private Map<String, String> labels;
}

labels will load on runtime.

We can manually load it without spring.

Sorry, the previous question was answered incorrectly, label is just a string, not json.

@nobodyiam
Copy link
Member

nobodyiam commented Oct 8, 2021

@Anilople

The implementation of the summer 2021 task is a little different from what we previously discussed:

  1. label name is system defined instead of user defined
    • e.g. user can only config apollo.label=xx instead of some customized names like my-label=xx
  2. the relationship between label and ip is OR instead of AND
    • e.g. the actual rule for the following screenshot is ip in ('1.2.3.4', '3.4.5.6') or label in ('a', 'b')
    • image

The above 2 differences might make the rules not that powerful but I think it's acceptable since it also makes the rules easy to maintain and understand. What do you think?

@Anilople
Copy link
Contributor

Anilople commented Oct 8, 2021

@Anilople

The implementation of the summer 2021 task is a little different from what we previously discussed:

  1. label name is system defined instead of user defined

    • e.g. user can only config apollo.label=xx instead of some customized names like my-label=xx
  2. the relationship between label and ip is OR instead of AND

    • e.g. the actual rule for the following screenshot is ip in ('1.2.3.4', '3.4.5.6') or label in ('a', 'b')
    • image

The above 2 differences might make the rules not that powerful but I think it's acceptable since it also makes the rules easy to maintain and understand. What do you think?

A single label also can implement the function which multiple custom labels have.

And it's acceptable and very easy to maintain and understand. Multiple custom labels seems a little complex.

I agree that.

nobodyiam
nobodyiam previously approved these changes Oct 9, 2021
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

# Conflicts:
#	CHANGES.md
# Conflicts:
#	CHANGES.md
@zcy1010
Copy link
Contributor Author

zcy1010 commented Oct 9, 2021

LGTM

Thank you very much for your review. I am very honored to choose Apollo in the Summer-2021. I really learned a lot during this event. I hope I can contribute to Apollo in the future.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit 67a78ef into apolloconfig:master Oct 9, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2021
@nobodyiam nobodyiam added this to the 2.0.0 milestone Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

添加自定义灰度,不单单依靠IP来推送灰度规则
4 participants