Skip to content

Add sudo to docker role#177

Open
vvaldez wants to merge 1 commit intorhtconsulting:openshift-enterprise-3from
vvaldez:docker-needs-sudo
Open

Add sudo to docker role#177
vvaldez wants to merge 1 commit intorhtconsulting:openshift-enterprise-3from
vvaldez:docker-needs-sudo

Conversation

@vvaldez
Copy link
Contributor

@vvaldez vvaldez commented Jun 6, 2016

What does this PR do?

Adds ansible_sudo: true to docker role. Without this, docker will fail to install using a non-root user in an image such as the rhel-guest-image.

How should this be manually tested?

Run ose-provision.yml using an image that does not use the user root such as the rhel-guest-image

Is there a relevant Issue open for this?

None.

Who would you like to review this?

/cc @oybed @sabre1041 @etsauer

@oybed
Copy link
Member

oybed commented Jun 6, 2016

@vvaldez this looks A_OK from a high level, but I thought there were additional changes needed to make this all work. Are the rest part of PR #168 that @etsauer is working?

@vvaldez
Copy link
Contributor Author

vvaldez commented Jun 6, 2016

That is correct, but last week I asked about submitting PRs against #168 and the consensus was to wait until it was submitted. I'm adding this PR because any of my current testing using the primary branch will fail without this. I'm more than happy to submit changes against #168 but don't want to slow down the acceptance of it.

In that PR there are different ways this is being overcome using the following at the play level:

become: true
become_method: sudo

But I didn't know if it would be best to make that change in a new PR or just add it at the connection level like I did here.

@etsauer
Copy link
Contributor

etsauer commented Jun 7, 2016

i'm ok with this being a separate PR.

I do want to say that our future goal should be to get all of this stuff out of the playbook and into hostvars that we set as part of inventory. The current playbook would require a user to change this to support hosts to which they want to ssh directly as root.

Since we've already been doing this in the playbook and are working to get the quick and dirty way done first, I'm ok merging this as-is, but we should additionally open up an issue to refactor all of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants