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.

Sign up for GitHub

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

Jump to bottom

Remove option to configure custom config file via CONF_FILE or -Des.default.conf #13772

Merged
merged 2 commits into from
Oct 7, 2015
Merged

Remove option to configure custom config file via CONF_FILE or -Des.default.conf #13772

merged 2 commits into from
Oct 7, 2015

Conversation

brwe
Copy link
Contributor

@brwe brwe commented Sep 24, 2015

It is rarely used and was not consistently handled by different distributions anyway.
This commit also adds a test for specifying CONF_DIR when installing plugins and
starting elasticsearch.

relates to #12712 and #12954
closes #5329
closes #13715

Would be great if someone with a windows machine could test the bat file, I unfortunately can't.

@brwe brwe mentioned this pull request Sep 24, 2015
Closed
rjernst
rjernst reviewed Sep 24, 2015
View reviewed changes
core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java
loadFromEnv = false;
output.loadFromPath(environment.configFile().resolve(System.getProperty("elasticsearch.config")));
}
}
if (loadFromEnv) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the local loadFromEnv and be removed?

@rjernst
Copy link
Member

rjernst commented Sep 24, 2015

LGTM, I left a couple very minor comments.

nik9000
nik9000 reviewed Sep 24, 2015
View reviewed changes
qa/vagrant/src/test/resources/packaging/scripts/plugin_test_cases.bash
@test "[$GROUP] start elasticsearch with a custom CONFIG_FILE and check failure" {
local CONF_FILE="$ESCONFIG/elasticsearch.yml"

if is_dpkg ; then
Copy link
Member

Choose a reason for hiding this comment

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

No space here I think.

@nik9000
Copy link
Member

nik9000 commented Sep 24, 2015

I also left some pretty minor comments.

@electrical
Copy link

Where does the systemd pre-exec file come from ?

edit: ignore me; found it now. seems i missed it when browsing on my mobile.

@brwe
Copy link
Contributor Author

brwe commented Sep 25, 2015

@nik9000 @rjernst thanks for the review! addressed all comments.
@electrical I added the file to check if CONF_FILE was defined and not start elasticsearch if so. I don't know systemd too well so if there is another way to have this check before we start elasticsearch I'd be happy to change that.

@clintongormley clintongormley changed the title remove option to configure custom config file via CONF_FILE or -Des.d… Remove option to configure custom config file via CONF_FILE or -Des.default.conf Sep 25, 2015
@clintongormley clintongormley added >breaking :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts labels Sep 25, 2015
@brwe brwe mentioned this pull request Sep 25, 2015
Closed
@brwe
Copy link
Contributor Author

brwe commented Sep 25, 2015

Chatted with @spinscale who wanted the check for -Des params to be moved from bin/elasticsearch script to Bootstrap. I added a commit for that too. I reordered logging conf initialization so that we get a proper message, not sure though if this is problematic?

@brwe brwe force-pushed the remove-CONF_FILE branch 2 times, most recently from 6998e65 to 988f445 Compare September 29, 2015 15:37
@brwe
Copy link
Contributor Author

brwe commented Sep 29, 2015

@rjernst or @nik9000 or @spinscale can you take another look?

@brwe
Copy link
Contributor Author

brwe commented Sep 29, 2015

Forgot to add that I tried out the service.bat on windows now too. Thanks @dliappis for the help!

@nik9000
Copy link
Member

nik9000 commented Oct 6, 2015

LGTM

@brwe
remove option to configure custom config file via CONF_FILE or -Des.d…
71aefd5 
…efault.conf

It is rarely used and was not consistently handled by different distributions anyway.
This commit also adds a test for specifying CONF_DIR when installing plugins and
starting elasticsearch.

relates to elastic#12712 and elastic#12954
closes elastic#5329
closes elastic#13715
@brwe brwe force-pushed the remove-CONF_FILE branch from 8d4bd94 to 71aefd5 Compare October 6, 2015 17:42
nik9000
nik9000 reviewed Oct 6, 2015
View reviewed changes
qa/vagrant/src/test/resources/packaging/scripts/packaging_test_utils.bash
# $1 expected status code
# $2 additional command line args
run_elasticsearch_service() {
# Set the CONF_DIR setting in case we start as a service
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably save $1 and $2 to named variables because they can get confusing lower down.

nik9000
nik9000 reviewed Oct 6, 2015
View reviewed changes
qa/vagrant/src/test/resources/packaging/scripts/packaging_test_utils.bash
elif is_sysvinit; then
run service elasticsearch status
[ "$status" -eq 0 ]
[ "$status" -eq $1 ]
Copy link
Member

Choose a reason for hiding this comment

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

This would be clearer is $1 were $expected_status or something.

@brwe
Copy link
Contributor Author

brwe commented Oct 6, 2015

@nik9000 addressed all comments. I made the timeout now dependent on if we run in background or not but my bash is that of a 3 year old so if you have any suggestion to improve readability don't hold back.

@nik9000
Copy link
Member

nik9000 commented Oct 6, 2015

LGTM

brwe added a commit that referenced this pull request Oct 7, 2015
@brwe
Merge pull request #13772 from brwe/remove-CONF_FILE
8a590d4 
Remove option to configure custom config file via CONF_FILE or -Des.default.conf
@brwe brwe merged commit 8a590d4 into elastic:master Oct 7, 2015
@brwe brwe mentioned this pull request Oct 7, 2015
Closed
brwe added a commit that referenced this pull request Oct 7, 2015
@brwe
remove option to configure custom config file via CONF_FILE or -Des.d…
d51c686 
…efault.conf

It is rarely used and was not consistently handled by different distributions anyway.
This commit also adds a test for specifying CONF_DIR when installing plugins and
starting elasticsearch.

relates to #12712 and #12954
closes #5329
closes #13715

Merge pull request #13772 from brwe/remove-CONF_FILE
brwe added a commit that referenced this pull request Oct 7, 2015
@brwe
remove option to configure custom config file via CONF_FILE or -Des.d…
bb45589 
…efault.conf

It is rarely used and was not consistently handled by different distributions anyway.
This commit also adds a test for specifying CONF_DIR when installing plugins and
starting elasticsearch.

relates to #12712 and #12954
closes #5329
closes #13715

Merge pull request #13772 from brwe/remove-CONF_FILE
brwe added a commit that referenced this pull request Oct 7, 2015
@brwe
remove option to configure custom config file via CONF_FILE or -Des.d…
0aa1ad0 
…efault.conf

It is rarely used and was not consistently handled by different distributions anyway.
This commit also adds a test for specifying CONF_DIR when installing plugins and
starting elasticsearch.

relates to #12712 and #12954
closes #5329
closes #13715

Merge pull request #13772 from brwe/remove-CONF_FILE
@brwe brwe mentioned this pull request Oct 7, 2015
Closed
@tlrx tlrx mentioned this pull request Nov 2, 2015
Closed
@majormoses majormoses mentioned this pull request Nov 3, 2015
Closed
@bstascavage
Copy link

This was actually a feature we used and this required me to re-architecture our cluster deployment. I have no idea why, in 2015, you would remove the ability to specify a config file, and instead for the user to have a config file be a specific filename. Obviously the damage is done, but I would dispute the claim that it is 'It is rarely used'

@dadoonet dadoonet mentioned this pull request Jan 29, 2016
Closed
@maneeshmp maneeshmp mentioned this pull request Jan 14, 2017
Closed
@rjernst rjernst mentioned this pull request Aug 14, 2017
Closed
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>breaking :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop support for CONF_FILE CONF_FILE in /etc/sysconfig not used by startup script on RedHat
7 participants
@brwe @rjernst @nik9000 @electrical @bstascavage @clintongormley @mark-vieira