fix: add network in linux installation script #1566
Conversation
Signed-off-by: charankamarapu <kamarapucharan@gmail.com>
keploy.sh
Outdated
| echo -n "Docker not found on device, please install docker and reinstall keploy if you are willing to use applications with docker" | ||
| return | ||
| fi | ||
| if ! docker info &> /dev/null; then |
There was a problem hiding this comment.
This will throw an error in case of Linux systems where docker is not added to user group because you are not using sudo
Also, please don't copy paste code, since this code is already present, just add it to a function with a check for sudo. Previously we did not have a check for sudo because in majority cases, we don't need sudo to run docker on macos
There was a problem hiding this comment.
Added two functions check docker status for Darwin as well as linux. Created separate functions because their logs are different.
There was a problem hiding this comment.
I think we can standardize that, instead of creating a seperate function just for that one thing.
|
|
||
| install_docker() { |
There was a problem hiding this comment.
why haven't you called add_network in this function? In macos, we still install keploy via docker, so it will be needed in that case. In the future when we add a binary for macos, we can remove the install_docker completely
There was a problem hiding this comment.
Added add_network in darwin
Signed-off-by: charankamarapu <kamarapucharan@gmail.com>
Signed-off-by: charankamarapu <kamarapucharan@gmail.com>
keploy.sh
Outdated
|
|
||
|
|
||
| install_docker() { | ||
| check_docker_status_for_Darwin() { | ||
| check_sudo | ||
| sudoCheck=$? | ||
| network_alias="" | ||
| if [ "$sudoCheck" -eq 0 ] && [ $OS_NAME = "Linux" ]; then |
There was a problem hiding this comment.
os name should be darwin here.
keploy.sh
Outdated
| network_alias="sudo" | ||
| fi | ||
| if ! $network_alias which docker &> /dev/null; then | ||
| echo -n "Docker not found on device, please install docker and reinstall keploy if you are willing to use applications with docker" |
There was a problem hiding this comment.
The logs for mac and linux shouldn't be so different in my opinion. and I think "willing" doesn't fit well in a log message. I think just change both to "Docker not found on device. Please install docker and reinstall keploy". Same thing for starting docker.
There was a problem hiding this comment.
we shouldn't force user to use docker on linux if he has no application with docker
keploy.sh
Outdated
| echo -n "Docker not found on device, please install docker and reinstall keploy if you are willing to use applications with docker" | ||
| return | ||
| fi | ||
| if ! docker info &> /dev/null; then |
There was a problem hiding this comment.
I think we can standardize that, instead of creating a seperate function just for that one thing.
Signed-off-by: charankamarapu <kamarapucharan@gmail.com>
|
Regarding - "standardize thing, instead of creating a seperate function just for that one thing." I think this is the standard way, branching should happen in the parent function only. You can not use same conditions in the child functions also which you have used in parent. Then there is no purpose of branching in parent. and more over they are not identical the log messages are different. |
Signed-off-by: charankamarapu <kamarapucharan@gmail.com>
Related Issue
Closes: #1565
Describe the changes you've made
A clear and concise description of what you have done to successfully close your assigned issue. Any new files? or anything you feel to let us know!
Type of change
Please let us know if any test cases are added
Please describe the tests(if any). Provide instructions how its affecting the coverage.
Describe if there is any unusual behaviour of your code(Write
NAif there isn't)A clear and concise description of it.
Checklist:
Screenshots (if any)