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

profiles support global env value #29657

Closed
xiaomizhou2 opened this issue Jan 5, 2024 · 13 comments · Fixed by #29909
Closed

profiles support global env value #29657

xiaomizhou2 opened this issue Jan 5, 2024 · 13 comments · Fixed by #29909

Comments

@xiaomizhou2
Copy link

xiaomizhou2 commented Jan 5, 2024

Feature Request

For English only, other languages will not be accepted.

Please pay attention on issues you submitted, because we maybe need more details.
If no response anymore and we cannot make decision by current information, we will close it.

Please answer these questions before submitting your issue. Thanks!

Is your feature request related to a problem?

no;

Describe the feature you would like.

sharding yaml file context support global env value.example:

dataSources:
  master:
        url: ${DATASOUCE_URL:jdbc:mysql:127.0.0.1:3306/test}

I can modify it with env.

Some data, such as username, password, can be written in ciphertext.

Thanks!

@linghengqian
Copy link
Member

  • This only requires reading the YAML content when implementing the org.apache.shardingsphere.driver.jdbc.core.driver.ShardingSphereURLProvider SPI, and then replacing it with system variables.

  • The SPI of org.apache.shardingsphere.driver.jdbc.core.driver.ShardingSphereURLProvider provides the ability to dynamically read YAML files, but after reading the file, the configuration will still overwrite the metadata of the local or remote registry according to the configuration of Mode Config. For a simple example, refer to https://github.com/apache/shardingsphere-plugin/tree/main/jdbc%2Fdriver%2Fapollo .

  • This is actually a hidden subtask of Improve GraalVM Reachability Metadata and corresponding nativeTest related unit tests #29052 , because the integration of testcontainers-java cannot always use a fixed port, and a dynamic YAML configuration method needs to be provided.

  • Considering that the Christmas holidays are not over yet, does your raising of the current issue mean that you are ready to submit a PR?

@xiaomizhou2
Copy link
Author

I can try to implement these features.

@xiaomizhou2
Copy link
Author

xiaomizhou2 commented Jan 9, 2024

@linghengqian Hello,I have completed the handling of global variables. PR:https://github.com/apache/shardingsphere/pull/29682

@linghengqian
Copy link
Member

  • The most embarrassing thing for me is that I will not be back from my Christmas holiday until next Saturday, so I may not have time to comment on the PR. I noticed that the SPI involved in the relevant PR lacks a document, and I may add the corresponding document in the near future.

  • The PR did not pass the code formatting validation. You can execute first ./mvnw spotless:apply -Pcheck -T1C. Then execute ./mvnw checkstyle:check -Pcheck -T1C to manually adjust to repair the CI.

@xiaomizhou2
Copy link
Author

Okay, I'll fix it.

@xiaomizhou2
Copy link
Author

@linghengqian I've submitted a new pull request. Thanks. #29704

@xiaomizhou2
Copy link
Author

Let's see if you have any other solutions to solve this problem.

@linghengqian
Copy link
Member

Let's see if you have any other solutions to solve this problem.

  • I need some time to think about the design. There are some optional symbols to replace ${}.

@linghengqian
Copy link
Member

linghengqian commented Jan 28, 2024

  • To be honest, I hit a limitation of Java itself while trying to write a unit test for System.getenv(). Of course I know that System.getenv() almost certainly serves the usage process of Docker Image.
    • @org.junitpioneer.jupiter.SetEnvironmentVariable of org.junit-pioneer:junit-pioneer:2.2.0 requires additional --add-open for maven-surefire-plugin on JDK21, which is something I hope to avoid at this stage because it involves questionable changes on the JDK22-ea milestone.
    • uk.org.webcompere:system-stubs-jupiter:2.1.6 will break ShardingSphere's unit test CI for JDK8 because it uses JDK11's bytecode.
    • com.github.stefanbirkner:system-rules:1.19.0 and com.github.stefanbirkner:system-lambda:1.2.1 will break ShardingSphere's unit test CI for JDK17+. They lack maintenance.
    • For Mockito 4, I personally don't think the method of Mockito.spy is appropriate, and doing Mockito.spy on ShardingSphereURLManager is too weird. Of course we can do this, but unit testing will look a lot more obscure.
    • It's hard for me to imagine that there will be a good way to simulate environment variables on JDK8-JDK21 before ShardingSphere improves the JDK baseline to JDK11. Yes, the runtime requirement for ShardingSphere has always been JDK8.
  • Rather than using Environment Variables(System.getenv()), I personally prefer to use System Property Variables(System.getProperty()). I anticipate adjusting the final outcome in this direction. Another idea is to set Args/buildArgs directly on the ShardingSphere URL string.
  • The regular expression of \$\$\{(.+?)\}$ also looks good. Although more can be restricted. Expecting a match like
dataSources:
  ds_0:
    dataSourceClassName: com.zaxxer.hikari.HikariDataSource
    driverClassName: org.h2.Driver
    jdbcUrl: $${FIXTURE_JDBC_URL::jdbc:h2:mem:foo_ds_do_not_use}
    username: sa
    password:
  • I'm still organizing a PR for this issue. But my vacations are always full of fun things to distract me.

@linghengqian
Copy link
Member

@xiaomizhou2
Copy link
Author

xiaomizhou2 commented Feb 23, 2024

@linghengqian I just finished my Chinese New Year holiday.I prefer using the ENV() function to replace global variables. While ${} is already a keyword in shardingsphere, $${} can also be used, but I feel it may lead to ambiguity.

@linghengqian
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment