Generate node roles instead of hard coding them

[Crazybus]

No description provided.

[jordansissel]

Generate node names instead of hard coding them

Commit message probably should say 'node roles' not 'node names'.

[rendhalver]

I am not sure this is worth it.
There are only three roles from what I can tell.
This also doesn't seem to check whether the specified role is a role that can exist.

[Crazybus]

Commit message probably should say 'node roles' not 'node names'.

Nice catch! I updated the commit message.

[Crazybus]

@rendhalver

I am not sure this is worth it.
There are only three roles from what I can tell.

There is also node.ml which is used in the test. Part of the reason I'm not hardcoding them is it saves us from having to add in extra version logic when new roles are added in the future.

This also doesn't seem to check whether the specified role is a role that can exist.

Elasticsearch already does this validation. If you set a node role that doesn't exist it will fail to start up.

org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: unknown setting [node.blah] please check that any required plugins are installed, or check the breaking changes documentation for removed settings

Do you see any issues with doing it this way?

[rendhalver]

Oh ok I wasn't aware there were more than those three node types. :)

I am just concerned about deploying broken clusters or breaking current clusters.
Can we make a note in the docs with the list of acceptable node types?

[jordansissel]

Can we make a note in the docs with the list of acceptable node types?

I am +1 on this proposal, but I think it's outside the scope of this PR given this concern exists in both master and this PR. The existing docs in master say "A hash map with the specific roles for the node group" and links to the Elasticsearch docs on node roles. The statement is still true even with this PR, so it doesn't seem to be changing much about the user experience.

I am +1 on input validation and improving feedback when typos/errors are made, but I think that can be improved outside of this PR.

[jordansissel]

I tested this, and it works for me.

I'm unable to run make test though:

% make -C elasticsearch test
...
 ---> 14b8335fab5e
Successfully built 14b8335fab5e
Successfully tagged helm-tester:latest
docker run --rm -i --user "$(id -u):$(id -g)" -v $(pwd)/../:/app -w /app/$(basename $(pwd)) helm-tester make template
docker run --rm -i --user "$(id -u):$(id -g)" -v $(pwd)/../:/app -w /app/$(basename $(pwd)) helm-tester make lint
docker run --rm -i --user "$(id -u):$(id -g)" -v $(pwd)/../:/app -w /app/$(basename $(pwd)) helm-tester make pytest
make: stat: Makefile: Permission denied

The above failure seems related to my environment and not specific to this PR (as it fails also on master)

[jordansissel]

Tested manually and reviewed the changes.

LGTM :)

[Crazybus]

The above failure seems related to my environment and not specific to this PR (as it fails also on master)

Weird, I thought that maybe specifying the -C elasticsearch for the directory was breaking it. But that also works just fine for me. What happens if you run make lint? I'm curious whether it is the "run inside docker" stuff or just general makefile weirdness.

Can you also show me the output of make --version?