Add explicit docker-entrypoint to comply with official image rules

[l-lotz]

This is based on #9 with a more sophisticated check, but without the ".travis.yml" and "Makefile". Also changed install of "openssh" to "openssh-client" to only install the client.

[oliv3r]

To shrink the image further, dropbear-ssh could be used instead of openssh-client; (12kb vs 3mb) sadly, the known-hosts file's are not entirely compatible I think. The hostnames are hashed on later versions of openssh-client. The same problem of course applies when mixing older/newer versions of openssh-client's so not sure how much of a dealbreaker that would be ...

Maybe with a -slim variant dropbear-ssh would be more favorable?

[oliv3r]

Suggested change

	exec "$@"
	exec "${@}"

[l-lotz]

Thanks for the review @oliv3r. I added all your suggested changes. I think your slim variant is also a good idea. Maybe it would make sense to provide a "openssh" and a "dropbear" variant/tag, and make the "openssh" variant also the "latest". The Readme could then add an explanation, that there are differences in the known hosts format, and that the dropbear variant is smaller.

[oliv3r]

Yeah, thinking about the slim variant, it should probably even leave out the ssh client, as not everybody may need it (with CI, you can just as easily use https for example.

The normal one, would have a full blown openssh-client (+latest) as you suggest, and then the 'light (-dropbear or -light?) could have dropbear instead.
I can imagine for a lot of CI users, having a dropbear variant is more then enough (having 2 options, either allowing all hosts, e.g. no known_hosts; or having a predifined known_hosts, at which point you can predefined the correct known_hosts).

As for 'regular' users they almost always will just use the 'latest' which will have the normal openssh-client.

But lets get this PR merged first, it's been idling since dec. 2018 and it's brother since sept.
Is this repo still being maintained updated? What can we do to get a PR merged?

[oliv3r]

P.S. I do recommend doing a git rebase -i origin/master on your branch, squash or fixup the edits and then push -f so that we keep a clean commit history.

[l-lotz]

@oliv3r I cleaned up the git history.
@muppet3000 could you check if this addresses the concerns you voiced in #9 ?

[muppet3000]

Do we know if there's a publicly available docker build of this branch when you guys are making commits? I can make my system pull it and test it if there is.

[l-lotz]

I've created lilotz/git on dockerhub to build from my branch.

[oliv3r]

and I did @ https://hub.docker.com/r/olliver/alpine-git but I think l-lotz's build is probably more up to date :)

[oliv3r]

Not sure what the official image rules stance is of VOLUME, but I was having trouble with it (btrfs, I know, my own fault). But what is the advantage and purpose of having the VOLUME in this container?

The workdir I can understand somewhat, so that the we use a guaranteed dir, but the VOLUME I'm not sure the purpose. You still have to volume mount files to get into the image, if you use -v :/git you are copying/storing the files in the volume (to what end) when all you want is to use the git commands.

So for a normal user it makes no little sense, for a CI it adds no value at all.

So in that line, would it make sense to remove the VOLUME from the Dockerfile as part of this PR to be a more compliant container?

[oliv3r]

i should have mentioned this before, but as I forgot to run this script through shellcheck.net, it also mentions a few minor issues here.
command -v is preferred over which
== is undefined in POSIX sh
and
&& is preferred over -a.

Suggested change

	if [ "${#}" -gt 0 -a "${1#-}" == "${1}" -a ! -x "/usr/libexec/git-core/git-${1}" ] && which "${1}" > /dev/null 2>&1 ; then
	if [ "${#}" -gt "0" ] && \
	[ "${1#-}" = "${1}" ] && \
	[ ! -x "/usr/libexec/git-core/git-${1}" ] && \
	command -v "${1}"; then

[oliv3r]

for alignment consistency probably a good idea to also indent with 4 to stay in sync with the code above.

[oliv3r]

any update on this?

[l-lotz]

@oliv3r I included your suggestions

[oliv3r]

@l-lotz thanks :) but the maintainer is not moving forward much is it?

[ozbillwang]

Thanks @oliv3r & @l-lotz

Currently I can't find out a better way to do unit test for this and future changes, have to hold it.

It looks, this image is using widely currently, which I am not supposed to.

[muppet3000]

@oliv3r & @l-lotz Apologies for the intermittent interations on this. These changes look like they cover everything they need to to address the issue #8 , plus, as far as I can tell it's backwards compatible.

@ozbillwang - With regards to your comments, I don't understand what you're saying, are you saying you won't merge this until you find a way to unit test?
The previous version was released without testing and is widely used, I don't think it's a justifiable reason to hold off on merging this request. If you're worried about current users losing existing capability then you should look to tag the existing version as "previous/stable" and then releasing this version as the new default ('latest' tag) in dockerhub. @l-lotz @oliv3r do you agree?

[l-lotz]

@muppet3000 I think it would make sense to still have access to the old version, but I would use some kind of versioning schema to be able to keep a specific version, even when "stable" or "previous" changes. To be fair, compared to the changes in this pull request, the last change was rather small.

[oliv3r]

@muppet3000 having the old one as previous/stable makes sense. From what I can see, there should not be (m)any backwards compatibility breaking changes.

I've created #14 to add overall unit tests to ensure future stability.

[ozbillwang]

@l-lotz could you rebase from master?

[l-lotz]

@ozbillwang I've rebased it.

[jakern]

This did not work for me as is. PR in upstream l-lotz#1