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

Rename CONF_DIR to ES_PATH_CONF #26154

Closed
jasontedor opened this issue Aug 11, 2017 · 5 comments · Fixed by #26197
Closed

Rename CONF_DIR to ES_PATH_CONF #26154

jasontedor opened this issue Aug 11, 2017 · 5 comments · Fixed by #26197
Assignees
Labels
blocker >breaking :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v6.0.0-beta2

Comments

@jasontedor
Copy link
Member

The environment variable CONF_DIR was previously inconsistently used in our packaging to customize the location of Elasticsearch configuration files. The importance of this environment variable has increased starting in 6.0.0 as it's now used consistently to ensure Elasticsearch and all secondary scripts (e.g., elasticsearch-keystore) all use the same configuration. The name CONF_DIR is there for legacy reasons yet it's too generic. This issue proposes renaming CONF_DIR to ES_PATH_CONF for 6.0.0. This would be a breaking change but it's a smaller break now than it would be in the future after this environment variable becomes baked-in through the ecosystem.

@jasontedor jasontedor added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts discuss >breaking labels Aug 11, 2017
@nik9000 nik9000 added the v6.0.0 label Aug 11, 2017
@danielmitterdorfer
Copy link
Member

We've discussed it in Fix-It Friday and the change and the proposed name both make sense.

@rjernst
Copy link
Member

rjernst commented Aug 14, 2017

Copying my comment from the PR:

I understand the reasoning for calling it ES_PATH_CONF (to match the former --path.conf and/or path.conf settings), but since this is a new env var, I think we should make it make sense on its own. I also think using "path" is confusing, as we used to have the env var for the ES config file, so I would keep "dir" in the name. Thus I propose ES_CONF_DIR.

@jasontedor
Copy link
Member Author

I understand the reasoning for calling it ES_PATH_CONF (to match the former --path.conf and/or path.conf settings)

A primary reason is to match the naming convention for the existing settings: path.data, path.home, and path.logs and to match the name of the system property: es.path.conf which was also chosen to match these settings albeit for the es. prefix.

I also think using "path" is confusing, as we used to have the env var for the ES config file, so I would keep "dir" in the name.

I'm not sure what you mean about "path" being confusing, "path" is a perfectly reasonable synonym for "directory", shortened to "dir".

as we used to have the env var for the ES config file

I'm not sure what you're referring to here?

Given the above, I feel strongly that ES_PATH_CONF is the right name.

@rjernst
Copy link
Member

rjernst commented Aug 14, 2017

as we used to have the env var for the ES config file

I'm not sure what you're referring to here?

I was referring to the CONF_FILE env var that used to be supported, long long ago: #13772

@jasontedor
Copy link
Member Author

I was referring to the CONF_FILE env var that used to be supported, long long ago: #13772

That has not existed since 1.x, I don't think it should factor in here at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker >breaking :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v6.0.0-beta2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants