Closed bengroneman closed 3 years ago
@TheHackmeister Sorry for the confusing amount of file changes. I think this happened because I moved branches and ended up in some funky merge issues. please advise
Ah yeah, that's a good reminder. Some of this was already handled by the original installer, and it's one of the few bits that I haven't redone yet. I think you picked a good place for that logic to go.
There is more here that probably should be addressed here:
There are usually more steps to installing docker. I usually have to make it start when the system boots, initialize the swarm, and add the user to the docker group.
1 & 2 would be new code, but 3 & 4 can probably be copied from the original installer.
I worry this is more feedback than you were expecting. Don't feel like you have to implement it all or do so quickly. I'm happy to take care of whatever you're not interested in and if you run into issues or questions, I'm glad to help!
As far as the merge issues go, it looks like you have an extra commit. I'm not really sure what would have resulted in that, but the diffs came out looking reasonable, so I'm not worried about it. If it happens again, send me the relevant parts (probably git commands) from the output of history
so that I can try and duplicate the issue.
Thanks for working on this!
Awesome, thank you for the informative review, Spencer. I've made the adjustments as shown below. Let me know what you think!
I expect that most of the time TechnoCore will be ran as a user, which doesn't usually have the permissions needed to install docker. So we'll need figure out how to elevate our privilege to root. This stackoverflow had some cool snippets that might help. Perfect thank you for that resource. I believe this is handled by the docker install script. Maybe you can confirm this?
It would probably be good to give the option to quit rather than install docker.I gave the user 5 seconds to. Maybe it makes sense to make it simple with q instead of the standard ctrl-c.
Rather than have the docker-check.sh in the repo, I'm inclined to download it when needed. That way we don't have to maintain that code.Great point!
There are usually more steps to installing docker. I usually have to make it start when the system boots, initialize the swarm, and add the user to the docker group.Good point, I looked into the docker install script and it looks like this is already handled by the script. It probably makes sense for you to confirm this though
Hey Ben, Sorry for the slow response. I set up a rule to forward all GitHub mail to a specific mailbox, and then just didn't check that box for a while. Anyway, SFF stuff should be landing in my inbox now :D.
Hey Spencer, thank you for the comments. No worries about the slow response 👍
Let me know your thoughts, and thanks for all the help!
@bengroneman I finally got to the point where I wanted to run TechnoCore in the cloud and figured that'd be the perfect way to test this. So I'm merging your changes in and will only have to make a few small tweaks. Thanks for putting this together!
@TheHackmeister
That's great! I am so happy to have been a part of it!
Upon running this on a fresh installation of Fedora Linux I realized it no longer checked for an installation of docker. So I leveraged this StackOverflow QA, and this install script to get the job done.