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

feat: support environment variable in config file #745

Merged
merged 3 commits into from
Nov 22, 2021
Merged

feat: support environment variable in config file #745

merged 3 commits into from
Nov 22, 2021

Conversation

nic-6443
Copy link
Member

Sorry, the #713 closed because I wrongly rebase the upstream code, so now I have initiated a new PR. e2e has been added, can be review again

Please answer these questions before submitting a pull request

@nic-6443
Copy link
Member Author

@tao12345666333

@tao12345666333
Copy link
Member

Please resolve the conflicts, thanks!

…into patch-2

� Conflicts:
�	test/e2e/scaffold/k8s.go
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2021

Codecov Report

Merging #745 (2daf86c) into master (4a862e2) will increase coverage by 0.09%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #745      +/-   ##
==========================================
+ Coverage   31.70%   31.79%   +0.09%     
==========================================
  Files          66       66              
  Lines        6640     6652      +12     
==========================================
+ Hits         2105     2115      +10     
- Misses       4280     4281       +1     
- Partials      255      256       +1     
Impacted Files Coverage Δ
test/e2e/e2e.go 100.00% <ø> (ø)
pkg/config/config.go 65.00% <85.71%> (+2.50%) ⬆️

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 4a862e2...2daf86c. Read the comment docs.

@tao12345666333
Copy link
Member

The code LGTM. Thanks!

But I personally prefer to directly support configuration through environment variables or through configuration files
Instead of supporting environment variables in the configuration file.

WDYT? @gxthrj @tokers @lilien1010

@nic-6443
Copy link
Member Author

nic-6443 commented Nov 12, 2021

The code LGTM. Thanks!

But I personally prefer to directly support configuration through environment variables or through configuration files Instead of supporting environment variables in the configuration file.

WDYT? @gxthrj @tokers @lilien1010

@tao12345666333 In the case that the user chooses to use file configuration instead of commands of container, the use of environment variables is to write sensitive information through downward api and secret. So if the program directly uses env, we need to consider the problem of file configuration and env configuration merging, doesn't make this problem easy.

@tokers
Copy link
Contributor

tokers commented Nov 12, 2021

The code LGTM. Thanks!

But I personally prefer to directly support configuration through environment variables or through configuration files Instead of supporting environment variables in the configuration file.

WDYT? @gxthrj @tokers @lilien1010

Softwares like ETCD and Istio support using environment variables directly, and the precedence is highest, so it's my understanding that we do should support environment variables, configuration file and command line options.

@nic-6443
Copy link
Member Author

@tao12345666333 @tokers
Should we merge the current feature first, and add more options later?

@tokers
Copy link
Contributor

tokers commented Nov 19, 2021

@tao12345666333 @tokers Should we merge the current feature first, and add more options later?

Agree, or this PR might be too large to review.

@tao12345666333
Copy link
Member

Let's move forward, this feature also has some usage scenarios.

@tao12345666333 tao12345666333 added this to In progress in v1.4 Planning via automation Nov 19, 2021
@tao12345666333 tao12345666333 added this to the 1.4.0 milestone Nov 19, 2021
@tao12345666333 tao12345666333 added the enhancement New feature or request label Nov 19, 2021
@tao12345666333
Copy link
Member

ping @gxthrj @tokers for review & merge.

v1.4 Planning automation moved this from In progress to Reviewer approved Nov 20, 2021
Copy link
Member

@lilien1010 lilien1010 left a comment

Choose a reason for hiding this comment

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

🎉

@tao12345666333 tao12345666333 merged commit 9f2cd7f into apache:master Nov 22, 2021
v1.4 Planning automation moved this from Reviewer approved to Done Nov 22, 2021
Sindweller pushed a commit to Sindweller/apisix-ingress-controller that referenced this pull request Nov 25, 2021
@nic-6443 nic-6443 deleted the patch-2 branch April 3, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants