fix: confusion when substituting ENV in config file#11545
Conversation
shreemaan-abhishek
left a comment
There was a problem hiding this comment.
LGTM, but the test cases are sophisticated. I would suggest you to simplify if you can, but after a close look I understood them.
shreemaan-abhishek
left a comment
There was a problem hiding this comment.
there is a failing test case related to your change: https://github.com/apache/apisix/actions/runs/10667500821/job/29968905933?pr=11545#step:7:4777
| 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 |
There was a problem hiding this comment.
this could lead to a breaking change?
There was a problem hiding this 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.
## Description Fix confusion when substituting ENV in config files. The PR will be ported from apache#11545 and will cause related test cases to fail, so copy the test case files to EE first. ## Checklist - [x] I have explained **WHY** we need this PR and **HOW** the problem it solves - [x] The title of the PR concisely describes the code modification and complies with APISIX specifications. - [x] I have explained the changes or the new features added to this PR - [ ] I have added **e2e test cases** corresponding to this change - [ ] I have updated the documentation to reflect this change **NOTE: This PR will not be reviewed and merged until the contents of these checklists are completed.**
## Description Fix confusion when substituting ENV in config files. The PR will be ported from apache#11545. ## Checklist - [x] I have explained **WHY** we need this PR and **HOW** the problem it solves - [x] The title of the PR concisely describes the code modification and complies with APISIX specifications. - [x] I have explained the changes or the new features added to this PR - [x] I have added **e2e test cases** corresponding to this change - [ ] I have updated the documentation to reflect this change **NOTE: This PR will not be reviewed and merged until the contents of these checklists are completed.**
Description
Fixes #11121
Fixes #11239
init_by_luaphaseChecklist