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: custom server file location #3401

Merged

Conversation

Anilople
Copy link
Contributor

What's the purpose of this PR

Let user can custom file server.properties's location through system property or environment variable.

Which issue(s) this PR fixes:

#2734

#3389

Brief changelog

Resolve the path of file server.properties in com.ctrip.framework.foundation.internals.constant.PathConstants.ResolvedPaths#SERVER_PROPERTIES,
and change com.ctrip.framework.foundation.internals.provider.DefaultServerProvider to use it directly.

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.

@codecov-io
Copy link

codecov-io commented Nov 28, 2020

Codecov Report

Merging #3401 (7f25ef3) into master (c2a8726) will increase coverage by 0.03%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3401      +/-   ##
============================================
+ Coverage     51.51%   51.54%   +0.03%     
- Complexity     2307     2312       +5     
============================================
  Files           441      441              
  Lines         13762    13774      +12     
  Branches       1400     1404       +4     
============================================
+ Hits           7089     7100      +11     
- Misses         6185     6186       +1     
  Partials        488      488              
Impacted Files Coverage Δ Complexity Δ
...tion/internals/provider/DefaultServerProvider.java 65.11% <80.00%> (+2.95%) 20.00 <5.00> (+4.00)
...ervice/service/ReleaseMessageServiceWithCache.java 87.05% <0.00%> (+1.17%) 25.00% <0.00%> (+1.00%)

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 c2a8726...7f25ef3. Read the comment docs.

String path = Utils.isOSWindows() ? SERVER_PROPERTIES_WINDOWS : SERVER_PROPERTIES_LINUX;

File file = new File(path);
File file = ResolvedPaths.SERVER_PROPERTIES.toFile();
Copy link
Member

Choose a reason for hiding this comment

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

How about we make it a little simpler?

e.g.

  ...
  File file = new File(getServerPropertiesPath());
  ...

  String getServerPropertiesPath() {
    String serverPropertiesPath = getCustomizedServerPropertiesPath();

    if (Strings.isNullOrEmpty(serverPropertiesPath)) {
      serverPropertiesPath = Utils.isOSWindows() ? SERVER_PROPERTIES_WINDOWS : SERVER_PROPERTIES_LINUX;
    }

    return serverPropertiesPath;
  }

  private String getCustomizedServerPropertiesPath() {
    // 1. Get from System Property
    String serverPropertiesPath = System.getProperty("apollo.serverPropertiesPath");
    if (Strings.isNullOrEmpty(serverPropertiesPath)) {
      // 2. Get from OS environment variable
      serverPropertiesPath = System.getenv("APOLLO_SERVERPROPERTIESPATH");
    }

    return serverPropertiesPath;
  }

Copy link
Contributor Author

@Anilople Anilople Nov 28, 2020

Choose a reason for hiding this comment

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

Has Changed to simpler implemention.

How about use APOLLO_PATH_SERVER_PROPERTIES to instead of APOLLO_SERVERPROPERTIESPATH for better readability?

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 8be6348 into apolloconfig:master Nov 29, 2020
@nobodyiam nobodyiam added this to the 1.8.0 milestone Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants