git icon indicating copy to clipboard operation
git copied to clipboard

Add explicit docker-entrypoint to comply with official image rules

Open l-lotz opened this issue 7 years ago • 19 comments

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.

l-lotz avatar Dec 07 '18 12:12 l-lotz

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 avatar Mar 25 '19 06:03 oliv3r

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.

l-lotz avatar Mar 25 '19 10:03 l-lotz

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 avatar Mar 25 '19 16:03 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.

oliv3r avatar Mar 25 '19 16:03 oliv3r

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

l-lotz avatar Mar 25 '19 16:03 l-lotz

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.

muppet3000 avatar Mar 25 '19 16:03 muppet3000

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

l-lotz avatar Mar 25 '19 17:03 l-lotz

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 avatar Mar 25 '19 19:03 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 avatar Mar 25 '19 19:03 oliv3r

any update on this?

oliv3r avatar Apr 02 '19 16:04 oliv3r

@oliv3r I included your suggestions

l-lotz avatar Apr 03 '19 09:04 l-lotz

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

oliv3r avatar Apr 04 '19 18:04 oliv3r

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.

ozbillwang avatar Apr 08 '19 04:04 ozbillwang

@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?

muppet3000 avatar Apr 08 '19 08:04 muppet3000

@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.

l-lotz avatar Apr 08 '19 15:04 l-lotz

@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.

oliv3r avatar Apr 09 '19 05:04 oliv3r

@l-lotz could you rebase from master?

ozbillwang avatar May 04 '20 01:05 ozbillwang

@ozbillwang I've rebased it.

l-lotz avatar May 04 '20 06:05 l-lotz

This did not work for me as is. PR in upstream https://github.com/l-lotz/git/pull/1

jakern avatar Nov 16 '22 16:11 jakern