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

add yaml support to apollo-client #1944

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

nobodyiam
Copy link
Member

@nobodyiam nobodyiam commented Feb 3, 2019

Fixes #653, #1113, #1321, #1488, #1585, #1698,

  1. support yaml config file transform to properties
  2. support yaml config file injection to Spring

Usage

Suppose you have a yml format namespace called 'application.yaml' as the following screenshot, then we could retrieve or inject the config just as the properties format in several ways.

image

1. API Usage

Config config = ConfigService.getConfig("application.yaml");

2. Spring Injection Usage

2.1 XML Config

<apollo:config namespaces="application.yaml" />

2.2 Java Config

@Configuration
@EnableApolloConfig({"application.yaml"})
public class AppConfig {}

3. Spring Boot Injection Usage

application.properties/bootstrap.properties content:

apollo.bootstrap.enabled = true
apollo.bootstrap.namespaces = application.yaml

@codecov-io
Copy link

codecov-io commented Feb 3, 2019

Codecov Report

Merging #1944 into master will increase coverage by 0.6%.
The diff coverage is 94.24%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #1944     +/-   ##
===========================================
+ Coverage     50.26%   50.86%   +0.6%     
- Complexity     1927     1979     +52     
===========================================
  Files           396      398      +2     
  Lines         12040    12175    +135     
  Branches       1224     1249     +25     
===========================================
+ Hits           6052     6193    +141     
+ Misses         5534     5524     -10     
- Partials        454      458      +4
Impacted Files Coverage Δ Complexity Δ
...trip/framework/apollo/internals/YmlConfigFile.java 66.66% <ø> (ø) 1 <0> (ø) ⬇️
...amework/apollo/internals/PropertiesConfigFile.java 65% <ø> (-1.67%) 9 <0> (-1)
.../framework/apollo/core/enums/ConfigFileFormat.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ip/framework/apollo/internals/DefaultInjector.java 68% <100%> (+1.33%) 3 <0> (ø) ⬇️
...apollo/spring/config/PropertySourcesProcessor.java 92.45% <100%> (+0.45%) 17 <1> (+1) ⬆️
...rip/framework/apollo/spi/DefaultConfigFactory.java 94.11% <100%> (+4.11%) 18 <8> (+7) ⬆️
...nals/PropertiesCompatibleFileConfigRepository.java 88.88% <88.88%> (ø) 7 <7> (?)
...m/ctrip/framework/apollo/util/yaml/YamlParser.java 94.73% <94.73%> (ø) 24 <24> (?)
...rip/framework/apollo/internals/YamlConfigFile.java 92.3% <95.65%> (+25.64%) 9 <8> (+8) ⬆️
... and 4 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 c8f84a6...b1ca961. Read the comment docs.

@coveralls
Copy link

coveralls commented Feb 3, 2019

Coverage Status

Coverage increased (+0.6%) to 54.628% when pulling b1ca961 on nobodyiam:yml-support into c8f84a6 on ctripcorp:master.

@nobodyiam nobodyiam force-pushed the yml-support branch 5 times, most recently from 63e003b to 532fa48 Compare February 6, 2019 07:52
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

LGTM and I left a comment for your information

/**
* @return the properties form of the config file or an empty properties if the content is invalid
*/
Properties asProperties();
Copy link
Member

@kezhenxu94 kezhenxu94 Feb 6, 2019

Choose a reason for hiding this comment

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

Is it really good to return an empty properties (instead of throwing) in case of the content is invalid? Then how do we tell that whether the content is empty or invalid. Isn't there such a situation that we need to do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is really a good question!

I think we need to give some thoughts into this...

Copy link
Member

Choose a reason for hiding this comment

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

My thought was throwing some exceptions like MalformedYamlException in case where the YAML document is invalid and returning empty properties in case where the YAML document is empty

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are right, so asProperties will now throw a runtime exception if the content cannot be transformed to properties

1. support yaml config file transform to properties
2. support yaml config file injection to Spring
@nobodyiam nobodyiam merged commit bf1b637 into apolloconfig:master Feb 12, 2019
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.

4 participants