Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

For #36 - Add support for creating optional elasticsearch user and group #75

Merged
merged 1 commit into from
Apr 16, 2016
Merged

For #36 - Add support for creating optional elasticsearch user and group #75

merged 1 commit into from
Apr 16, 2016

Conversation

jerrac
Copy link
Contributor

@jerrac jerrac commented Mar 18, 2016

Here's the pull request. :) Per #36 (comment)

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@gingerwizard
Copy link

@jerrac apologies some other build issues associated with neillturner/kitchen-ansible#7 resolving. Will test this once we resolve.

@gingerwizard
Copy link

@jerrac Can we simplify this to just be es_user_uid & es_group_id? If either are specified we apply them to the elasticsearch user?

Also i think its worth raising an issue to be able to specify the user name i.e. change it from elasticsearch. I suggest we assume this user exists - relying on other roles/playbook to create it.

@jerrac
Copy link
Contributor Author

jerrac commented Mar 21, 2016

@gingerwizard I'll switch to es_user_id and es_group_id.

If either are specified we apply them to the elasticsearch user?

So, get ride of es_optional_user: True|False, and just use es_user_id is defined or es_group_id is defined on the appropriate tasks?

Also i think its worth raising an issue to be able to specify the user name i.e. change it from elasticsearch. I suggest we assume this user exists - relying on other roles/playbook to create it.

Don't we already have that? es_user, and es_group in defaults/main.yml.

@gingerwizard
Copy link

@jerrac yes we do! thanks :) I was confused because your pull uses literal "elasticsearch" rather than es_user and es_group.

Agreed re the other changes

@gingerwizard
Copy link

@jerrac LGTM. I'll test it shortly.

I think its worth adding a quick server spec test. Maybe to the config-2x and config-1x suites. Are you happy to do this or shall i add?

@gingerwizard
Copy link

jenkins, test it

@jerrac
Copy link
Contributor Author

jerrac commented Mar 21, 2016

I haven't used kitchen yet. I remember hearing about it once and thinking I should learn it. Heh.

So, would adding tests for this just involve adding the vars to one of the playbooks in the test/integration directory?

@gingerwizard
Copy link

@jerrac The tests seemed to have failed. It appears you need explicit is defined checks. It also worth checking the parameters are defined and not empty.

Yes ideally a test would use the parameters in one of the test playbooks - probably config is best. Server spec can then be used confirm the user id and group id were set correctly after the role had been applied here.

@jerrac
Copy link
Contributor Author

jerrac commented Mar 21, 2016

I don't need to edit the config_spec.rb files, right?

@gingerwizard
Copy link

Yes you can add it there. It will then run for be tested for 1x and 2x.
http://serverspec.org/resource_types.html#user
Is probably what you want.

@jerrac
Copy link
Contributor Author

jerrac commented Mar 21, 2016

Would this work for config-2x/serverspec/default_spec.rb? (Since those are the values I set in the config.yml file.)

require 'config_spec'
require 'user_spec'
require 'group_spec'

describe 'Config Tests v 2.x' do
  include_examples 'config::init', "2.1.0"
end

describe group('elasticuser') do
  it { should exist }
end

describe group('elasticuser') do
  it { should have_gid 333 }
end

describe user('elasticuser') do
  it { should exist }
end

describe user('elasticuser') do
  it { should belong_to_group 'elasticuser' }
end

describe user('elasticuser') do
  it { should have_uid 333 }
end

btw, thanks for coaching me through this stuff. 😄

@jerrac
Copy link
Contributor Author

jerrac commented Mar 21, 2016

Or maybe I should stick those in config_spec.rb.

Speaking of that file, would this section cause the tests to fail if the user is not elasticsearch?

If so, should I keep the es_user, and es_group values as "elasticsearch" in the test?

@gingerwizard
Copy link

Hi
Put it in config_sepc and then it will run for 1.x and 2.x. You can drop the 2.x test then. I would keep the username as elasticsearch - we currently dont have code to create an alternative user just set the uid and guid.

@gingerwizard
Copy link

Hey @jerrac thanks for this. We will test this, we're just experiencing some issues with the build env not clearing up docker images properly. Once we've resolved this ill run it and hopefully merged. Please feel free to contribute elsewhere, its really appreciated.

@gingerwizard
Copy link

infra problems hopefully solved so without further ado...

jenkins, test it

@gingerwizard
Copy link

@jerrac in an effort to improve stability the role has changed which is now causing conflicts. Can you please resolve and repush?

@jerrac
Copy link
Contributor Author

jerrac commented Apr 4, 2016

@gingerwizard Erm, did those changes help? Or am I missing something?

@dliappis
Copy link
Contributor

jenkins, test it

1 similar comment
@dliappis
Copy link
Contributor

jenkins, test it

…p before installing elasticsearch.

Renamed vars, set the tasks to use the es_user and es_group vars in elasticsearch-optional-user.yml. Modifed README.md to list the es_user_id and es_group_id vars, made note that both vars are required if they are used. Removed the commented out instances of the vars in defaults/main.yml.

Added es_user, es_group, es_user_id, es_group_id vars to config testing.

Made conditionals use explicit 'is defined' test.

Add tests for es_user_id and es_group_id.

Update defaults/main.yml and test/integration/config.yml to match master plus test config additions.
@jerrac
Copy link
Contributor Author

jerrac commented Apr 13, 2016

I rebased to master. Did that help?

And after poking around Jenkins some more, I found where to view the errors that caused the failure. Which is what I was missing when I was checking things earlier. :\

@gingerwizard
Copy link

jenkins, test it

@gingerwizard
Copy link

@jerrac all tests pass! merging. Thanks for the contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants