Fix registry local_user functionalitity.

[stejanse]

Docker uses $HOME environment variable to determine docker.json location.
Having /root as as standard $HOME, local_user functionality is broken.

[davejrt]

I think a better approach is to introduce another param, with the default for ROOT that can be overridden if you're not using the root user.

[stejanse]

@davejrt Thanks for you response! What would be the use case for a home directory that does not match the user's home directory? Feels more or less like double configuration (both user and user's home) to me.

[davejrt]

I've not tested it with an alternate user, so I suppose I am looking to ensure that any edge cases are covered off where an execution using another user would break something.

[stejanse]

For now, using local_user simply does not work unless the user is privileged to write to /root. I do not see any reason to write the .docker/config.json to any other location than your standard $HOME. If you do, you have to override your $HOME every time you use docker pull or docker push.

If you want me to change the PR anyways as you suggested, I will. I suggest to keep it this way: making $HOME configurable will only introduce redundant configuration.

[shamil]

I've not tested it with an alternate user, so I suppose I am looking to ensure that any edge cases are covered off where an execution using another user would break something.

Tested with alternate user and currently it's broken, I can't upgrade to latest version due to exactly this issue. I agree with @stejanse no point to have $HOME configurable.

[davejrt]

I've tested this and don't have any issue with this code as such. I'm interested to see the rest of your workflow, are you then using something else to pull images onto the box using your local user? My testing didn't allow for the images to be pulled as the script is executed as root.

[shamil]

I've tested this and don't have any issue with this code as such. I'm interested to see the rest of your workflow, are you then using something else to pull images onto the box using your local user? My testing didn't allow for the images to be pulled as the script is executed as root.

Not sure I understand the question.

My use case is simple, having a user (other than root) to be able to pull/push images to/from registry.
Pulling / pushing happens outside of puppet workflow, puppet just used to configure the user with registry pull/push rights.

@davejrt does this make sense?

[davejrt]

It makes sense, but I don't know why you'd want to break your puppet workflow by doing half the job with Puppet, when it would make more sense to allow the user to push/pull images within the scope of the module.

[shamil]

It makes sense, but I don't know why you'd want to break your puppet workflow by doing half the job with Puppet, when it would make more sense to allow the user to push/pull images within the scope of the module.

Without going into detail of our architecture, briefly we have CI processes that are responsible to push images.

But regardless, how we do it or not, how it relates to the this issue. Currently configuring private registries with non-root is broken and we need to fix that. This PR seems like fix the original problem without breaking any other flow, or I misunderstood something?

@davejrt what should be done in your opinion to have this merged?

[davejrt]

I know it doesn't break anything else, but in my opinion it feels like this PR only adds half of the functionality required to be able to complete the whole job with Puppet, which is ideally what we want to support in our modules.

[shamil]

@davejrt yep I see what you mean. You need this when using docker::run for example. Currenlty it's required to not use the local_user param. Which is the desired flow, due to docker::run uses root user.

I think originally (before this repo forked the original one) support for local_user was added due to request to configure it independendly from docker::run. And it was working really well, since version 2.x (as far as I remember) this feature broken, and it hold people like me from upgrading to latest versions of this module.

I agree with you, we should fix this whole thing like you suggest, BUT first we maybe need to fix the regression that was introduced since version 2.x

[davejrt]

@shamil Thanks for pointing that out. I've just reviewed the changes and you're correct in that this was a breaking change that we introduced. Merging this now with an aim in the future to add something to complete the workflow with alternate users.

Not required at this time