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(cli): support reserved environment variable "APISIX_DEPLOYMENT_E… #8898

Merged
merged 15 commits into from
Mar 1, 2023

Conversation

An-DJ
Copy link
Contributor

@An-DJ An-DJ commented Feb 21, 2023

…TCD_HOST"

Description

The user can use env variable "APISIX_DEPLOYMENT_ETCD_HOST" to config the etcd.host.

Priority: env var > config.yaml

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

apisix/cli/file.lua Outdated Show resolved Hide resolved
t/cli/test_main.sh Outdated Show resolved Hide resolved
t/cli/test_main.sh Outdated Show resolved Hide resolved
@@ -115,6 +117,21 @@ end
_M.resolve_conf_var = resolve_conf_var


local function replace_by_reserved_env_vars(conf)
-- TODO: support more reserved environment variables
local v = getenv("APISIX_DEPLOYMENT_ETCD_HOST")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's an array, let's call it APISIX_DEPLOYMENT_ETCD_HOSTS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it is a better idea that the variable name matches the configuration item?

deployment:
    etcd:
       host:
          - xxx
=======
APISIX_DEPLOYMENT_ETCD_HOST

It's easier to be understood, and there is no need to add more detailed documents to describe this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 Good point.

exit 1
fi

out=$(APISIX_DEPLOYMENT_ETCD_HOST='["http://127.0.0.1:2333"]' make run 2>&1 || true)
Copy link
Member

Choose a reason for hiding this comment

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

Can you configure a connectable address and check if it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

It seems this feature is broken, when I tried "valid host in env" + "invalid host in conf" + "make run".

@An-DJ
Copy link
Contributor Author

An-DJ commented Feb 24, 2023

It seems this feature is broken, when I tried "valid host in env" + "invalid host in conf" + "make run".

@spacewander This case is covered by the latest commit and passed.
https://github.com/apache/apisix/pull/8898/files#diff-cb891f9ef4943580aa92827c7d725f3c717923cb85344cc6d8195d70f7ef01b0R236

@spacewander
Copy link
Member

It seems this feature is broken, when I tried "valid host in env" + "invalid host in conf" + "make run".

@spacewander This case is covered by the latest commit and passed. #8898 (files)

I spend some time playing with this feature locally. Look like when APISIX is running, it will read the env variable in a different path. The CI stops APISIX too early, so APISIX doesn't have time to read it.

@@ -51,6 +51,9 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary

# reserved environment variables for configuration
env APISIX_DEPLOYMENT_ETCD_HOST;
Copy link
Member

Choose a reason for hiding this comment

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

Can we find a way to cover it in the CI? So we don't need to spend time verifying it locally next time.

t/cli/test_main.sh Show resolved Hide resolved
t/cli/test_main.sh Show resolved Hide resolved
@spacewander spacewander merged commit 789e122 into apache:master Mar 1, 2023
@juzhiyuan juzhiyuan mentioned this pull request Mar 6, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants