You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "zhoujiexiong (via GitHub)" <gi...@apache.org> on 2024/09/02 12:51:03 UTC
[PR] fix: confusion when substituting ENV in config file [apisix]
zhoujiexiong opened a new pull request, #11545:
URL: https://github.com/apache/apisix/pull/11545
### Description
Fixes #11121
Fixes #11239
1. Import environment variables with the same prefix to Nginx using assignment style
2. In the `init_by_lua` phase
3. There will be confusion issues
```
use Test::Nginx::Socket 'no_plan';
master_on();
repeat_each(1);
our $prefix = "prefix";
our $longer = "longer";
$ENV{TEST_ENV_SAME_PREFIX} = $prefix;
$ENV{TEST_ENV_SAME_PREFIX_LONGER} = $longer;
add_block_preprocessor(sub {
my ($block) = @_;
if (!$block->request) {
$block->set_value("request", "GET /t");
}
my $config = $block->config // "";
$config .= <<_EOC_;
location = /t {
content_by_lua_block {
local prefix = os.getenv("TEST_ENV_SAME_PREFIX")
local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
assert(prefix == "$::prefix" and longer == "$::longer")
}
}
_EOC_
$block->set_value("config", $config);
});
run_tests();
__DATA__
=== TEST 1: assignment style, the PREFIX 1st - incorrect
--- main_config eval
qq/
env TEST_ENV_SAME_PREFIX=$::prefix;
env TEST_ENV_SAME_PREFIX_LONGER=$::longer;
/
--- http_config eval
qq/
init_by_lua_block {
local prefix = os.getenv("TEST_ENV_SAME_PREFIX")
local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
assert(prefix == "$::prefix" and longer == "$::prefix")
}
/
=== TEST 2: assignment style, the LONGER 1st - correct
--- main_config eval
qq/
env TEST_ENV_SAME_PREFIX_LONGER=$::longer;
env TEST_ENV_SAME_PREFIX=$::prefix;
/
--- http_config eval
qq/
init_by_lua_block {
local prefix = os.getenv("TEST_ENV_SAME_PREFIX")
local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
assert(prefix == "$::prefix" and longer == "$::longer")
}
/
=== TEST 3: declaration style, the LONGER 1st - correct
--- main_config
env TEST_ENV_SAME_PREFIX_LONGER;
env TEST_ENV_SAME_PREFIX;
--- http_config eval
qq/
init_by_lua_block {
local prefix = os.getenv("TEST_ENV_SAME_PREFIX")
local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
assert(prefix == "$::prefix" and longer == "$::longer")
}
/
=== TEST 4: declaration style, the PREFIX 1st - also correct
--- main_config
env TEST_ENV_SAME_PREFIX;
env TEST_ENV_SAME_PREFIX_LONGER;
--- http_config eval
qq/
init_by_lua_block {
local prefix = os.getenv("TEST_ENV_SAME_PREFIX")
local longer = os.getenv("TEST_ENV_SAME_PREFIX_LONGER")
assert(prefix == "$::prefix" and longer == "$::longer")
}
/
```
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
4. Always update the documentation to reflect the changes made in the PR.
5. Make a new commit to resolve conversations instead of `push -f`.
6. To resolve merge conflicts, merge master instead of rebasing.
7. Use "request review" to notify the reviewer after making changes.
8. Only a reviewer can mark a conversation as resolved.
-->
--
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
Re: [PR] fix: confusion when substituting ENV in config file [apisix]
Posted by "zhoujiexiong (via GitHub)" <gi...@apache.org>.
zhoujiexiong commented on code in PR #11545:
URL: https://github.com/apache/apisix/pull/11545#discussion_r1756199709
##########
t/cli/test_main.sh:
##########
@@ -426,7 +426,7 @@ tests:
make init
-if ! grep "env TEST_ENV=very-long-domain-with-many-symbols.absolutely-non-exists-123ss.com:1234/path?param1=value1;" conf/nginx.conf > /dev/null; then
+if ! grep "env TEST_ENV;" conf/nginx.conf > /dev/null; then
Review Comment:
No, because when rendering nginx.conf, apisix cli decides to insert the relevant env directives based on whether they exist or are valid. The declarative method is sufficient. The final reading still needs to use `os.getenv`, `env FOO=bar; `this method is not necessary and can be replaced by the declarative method to ensure compatibility.
--
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
Re: [PR] fix: confusion when substituting ENV in config file [apisix]
Posted by "nic-chen (via GitHub)" <gi...@apache.org>.
nic-chen merged PR #11545:
URL: https://github.com/apache/apisix/pull/11545
--
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
Re: [PR] fix: confusion when substituting ENV in config file [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #11545:
URL: https://github.com/apache/apisix/pull/11545#discussion_r1756184538
##########
t/cli/test_main.sh:
##########
@@ -426,7 +426,7 @@ tests:
make init
-if ! grep "env TEST_ENV=very-long-domain-with-many-symbols.absolutely-non-exists-123ss.com:1234/path?param1=value1;" conf/nginx.conf > /dev/null; then
+if ! grep "env TEST_ENV;" conf/nginx.conf > /dev/null; then
Review Comment:
this could lead to a breaking change?
--
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