You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/11/10 07:07:45 UTC
[GitHub] [apisix-ingress-controller] nic-6443 opened a new pull request #745: feat: support environment variable in config file
nic-6443 opened a new pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745
Sorry, the https://github.com/apache/apisix-ingress-controller/pull/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
- Why submit this pull request?
- [ ] Bugfix
- [x] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- Related issues
#710
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] nic-6443 edited a comment on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
nic-6443 edited a comment on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-966889636
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-966835293
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#745](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2daf86c) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/4a862e206602ae9c7ac534fdfd9a557748b9ad26?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a862e2) will **increase** coverage by `0.08%`.
> The diff coverage is `85.71%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #745 +/- ##
==========================================
+ Coverage 31.70% 31.78% +0.08%
==========================================
Files 66 65 -1
Lines 6640 6651 +11
==========================================
+ Hits 2105 2114 +9
- Misses 4280 4281 +1
- Partials 255 256 +1
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/config/config.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbmZpZy9jb25maWcuZ28=) | `65.00% <85.71%> (+2.50%)` | :arrow_up: |
| [test/e2e/e2e.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dGVzdC9lMmUvZTJlLmdv) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4a862e2...2daf86c](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] nic-6443 commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
nic-6443 commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-973870852
@tao12345666333 @tokers
Should we merge the current feature first, and add more options later?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 merged pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
tao12345666333 merged pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-966882948
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-973918347
Let's move forward, this feature also has some usage scenarios.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-973919024
ping @gxthrj @tokers for review & merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-966904285
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] nic-6443 commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
nic-6443 commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-966889636
> 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
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter edited a comment on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-966835293
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#745](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2daf86c) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/4a862e206602ae9c7ac534fdfd9a557748b9ad26?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a862e2) will **increase** coverage by `0.09%`.
> The diff coverage is `85.71%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [test/e2e/e2e.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dGVzdC9lMmUvZTJlLmdv) | `100.00% <ø> (ø)` | |
| [pkg/config/config.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbmZpZy9jb25maWcuZ28=) | `65.00% <85.71%> (+2.50%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4a862e2...2daf86c](https://codecov.io/gh/apache/apisix-ingress-controller/pull/745?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] nic-6443 commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
nic-6443 commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-964843331
@tao12345666333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
tao12345666333 commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-966805428
Please resolve the conflicts, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on pull request #745: feat: support environment variable in config file
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #745:
URL: https://github.com/apache/apisix-ingress-controller/pull/745#issuecomment-973899803
> @tao12345666333 @tokers Should we merge the current feature first, and add more options later?
Agree, or this PR might be too large to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org