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

Added become statements and http(s) proxy support #4

Closed
wants to merge 2 commits into from

Conversation

mprasil
Copy link

@mprasil mprasil commented Feb 22, 2016

I've added become statements for the tasks in case we're not running Ansible as root and not using sudo.
Second change enables http(s) proxy setting for installations behind proxy.

@DavidWittman
Copy link
Owner

Hey @mprasil... Sorry it took so long to get to this, I just realized I didn't have the proper notifications set up for this repository.

I don't think adding become to every task is the right way to handle this. Instead, this should be handled at the play level in one of the following ways:

# Add become just for the docker role
- hosts: all
  remote_user: bob
  roles:
    - role: DavidWittman.docker
      become: yes
# Add become for all roles and tasks
- hosts: all
  remote_user: bob
  become: yes
  roles:
    - role: DavidWittman.docker

I'm not 100% certain that this is the "accepted" way to do it, but it gives more control to the end-user and prevents needless repetition of the become argument for each task. Happy to hear any additional feedback which you might have here.

With regard to your proxy changes, I'll gladly accept those if you open a new PR for them. I've even opened #5 to track this.

@mprasil
Copy link
Author

mprasil commented Mar 9, 2016

Hey, just noticed your updates. As for the become statements, I kinda like to include it in role, cause not every task might need root (or other) privileges (in this case it does) and it also works out of the box for people using the role without extra statements, but either approach is fine for me - I didn't see either being the "official" best practice.

That's also part of the reason for submitting this as two separate commits, so that you could perhaps cherry pick, but I see you went with other more generic solution.

@DavidWittman
Copy link
Owner

I'm going to search through some other roles tonight to see how this is
commonly handled. If I find that using become within the tasks of the role
is preferred I'll cherry pick your commit.

I was exploring changes for the http proxy and thought it would also be
useful to set NO_PROXY too. At which point I figured it would be prudent to
plan for any possible environment variables which the user wants to set, so
docker_env seemed like a convenient resolution. I apologize for not first
waiting for your response before making these changes, but they seemed
pretty sensible.
On Mar 9, 2016 2:45 PM, "mprasil" notifications@github.com wrote:

Hey, just noticed your updates. As for the become statements, I kinda like
to include it in role, cause not every task might need root (or other)
privileges (in this case it does) and it also works out of the box for
people using the role without extra statements, but either approach is fine
for me - I didn't see either being the "official" best practice.

That's also part of the reason for submitting this as two separate
commits, so that you could perhaps cherry pick, but I see you went with
other more generic solution.


Reply to this email directly or view it on GitHub
#4 (comment)
.

@mprasil
Copy link
Author

mprasil commented Mar 9, 2016

I don't think there's any consensus on this. From my limited experience, most just omit the become statements completely and leave that to the end user. I prefer to have it included for the reasons above, but I really don't care either way.

I like your approach with generic env variables, so no harm there. No need to apologize, just trying to help. ;)

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

Successfully merging this pull request may close these issues.

None yet

2 participants