Closed alex-golts closed 6 years ago
Thank you for your willingness to participate in the project. Basically: For each feature you should open an own pull request to make the features individually mergeable and discussable.
build_docker.sh
does not make any sense. First, the name of the target image is wrong (should be conform to the image name on Dockerhub). Second, there's already a Makefile. There's no reason to pollute the repository with a 5 word bashscript everybody should be able to write on his own (actually, this also applies to the Makefile. But Makefiles are more "standard" to build something).files/opt/nohup.out
: It's a really bad practice to commit log files to git repositories.run_docker.sh
? There's no documentation or any hint what this script is for.vpn.sh
does not make any sense for me. This script asks for the VPN's hostname. But f5fpc does that also on its own. This script has no advantage, so it's also just polluting the repository.
- why Ubuntu? I'm most familiar with it, I think I discovered I needed some tools like gedit, and couldn't find how to install them on alpine. But of course it's not necessary for everybody. I was hoping you would selectively take from this pull request whatever you find useful, doesn't have to be everything.
A running docker container is not supposed to be changed after it is created. Even having "state" in a container is not desired. Containers are all about "fire, destroy and forget". So, having text editors installed in a container is - following the rules of best practices - one of the most bad ideas you can have. Instead: Modify the image, kill old container, start new.
Also, personal preferences should not be part of open source. There should be some kind of objectively comparable things, like "this feature is only available in...". Since I do not need any special ubuntu features, the objective parameter I want to optimize is image size. I also prefer to use ubuntu, but for this project this does not make any sense at all.
- build_docker.sh - you're right, the makefile does exactly the same. I'm used to having a build_docker and run_docker script for every docker image that I maintain, but again a matter of taste. In this case the build script it's indeed trivial. I've had cases where I wanted the build script to do more stuff prior to the actual docker build command, that's where the practice is from.
Same as above, personal preferences should not be part of a software project. A Makefile is - from an objective point of view - more standard to build things instead of any custom build script.
- nohup.out - that's my mistake. I should've added files with this extension to .gitignore
- run_docker.sh - The hint is in the description I gave this pull request. This run script is meant to facilitate forwarding apps which use the X server (GUIs and such) which are run inside the docker, to the host's display (on Mac).
I'm not sure how to handle GUIs etc. in docker. I'm even not sure it if's a good idea. No real opinion about that. If you think that might be useful to others, you can create a pull request which enhances the README and gives an example how to enable GUI support (without an additional script).
It is also required if one wanted to use SSH inside the docker with the -X flag (i.e, with X forwarding). Normally this flag alone is enough but since here it's executed from within a docker container, it wouldn't work.
Nobody should want to use SSH within a docker container. See above.
- vpn.sh - I agree it doesn't do much. It's merely allowing one to type ./vpn.sh and then get asked the VPN server, instead of having to remember the command f5fpc -s -t .
You can also do f5fpc -s
. Then it will also ask for the server.
I think it's more intuitive but of course it's nothing major.
Currently I'm also trying to remove the python. I'm preparing a bash only script where you can choose between client and gateway mode:
Usage: ./f5fpc-vpn.sh <MODE> [<PARAMETERS...>]
Supported modes:
- client
- gateway
Supported parameters:
-h --help Show this help text
If you want to participate in that, I can push the branch. But for not it's not working yet.
P.S.: Done and merged to master ;) Would be nice if you could test that. Thanks!
Suggested branch based on Ubuntu. Also added display support for Mac