agile-learning-institute / mentorHub

9 stars 3 forks source link

mentorHub Desktop Edition search path errors #61

Closed MyNameIsAndrew-Mangix closed 8 months ago

MyNameIsAndrew-Mangix commented 9 months ago

Fix for silent fail on install script is needed. Currently, it appends to .bashrc. Depending on the .bashrc, it might append to a comment. The install doesn't recognize this, since the export snippet is present in the code. image This doesn't error. The script reads this and goes "ok, it's added, everything is fine". It doesn't get parsed by source ~/.bashrc

michquinn commented 9 months ago

Frankly, I'd rather the install script not modify ~/.bashrc at all and leave adding mh to PATH up to the user. A simple change allows the script to run regardless of whether it's in the user's PATH or not

diff --git a/mentorHub-developer-edition/mh b/mentorHub-developer-edition/mh
index ff6c675..4e1aeb4 100644
--- a/mentorHub-developer-edition/mh
+++ b/mentorHub-developer-edition/mh
@@ -2,7 +2,7 @@

 export COMMAND="$1"
 export NEW_PROFILE="$2"
-export INSTALL_PATH=$(dirname "$(which mh)")
+export INSTALL_PATH=$(dirname "$0")

 export IMAGES=($(docker compose -f "$INSTALL_PATH/docker-compose.yaml" --profile '*' config --images))
FlatBallFlyer commented 8 months ago

This issue is approved, and is now on-deck to be picked up. Create a PR with your proposed changes.

Please leave the append-to-path for .zshrc files if they exist. While you and some of the members are more than capable of making these edits, we do have members who are new to the shell, and who would be intimidated by editing their own rc file. Most of those users are Mac based users, so zsh is their default shell. I've considered doing a touch ~/.zshrc in the install script because some of our Mac users have never set up anything in their shell so the rc file isn't there. I don't want to do that to bash users, so I opted for a note in the install instructions to address that issue.

We do have members that are running a Linux VM for the first time, and had their first experience with bash just a few weeks ago. You will need to write up detailed instructions for them on how to add this to the search path.

Your change to INSTALL_PATH in the mh script appears to assume that you are in the directory where you want to look for configuration files. I don't think that is a safe assumption, which is why I used which. That variable is used to locate configuration and log files which are in the install folder.

FlatBallFlyer commented 8 months ago

This might be a good solution for this problem: https://github.com/agile-learning-institute/mentorHub/issues/54

FlatBallFlyer commented 8 months ago

Also - address this issue at the same time

68

michquinn commented 8 months ago

Your change to INSTALL_PATH in the mh script appears to assume that you are in the directory where you want to look for configuration files.

It does no such thing. "$0" resolves to the path of the script itself, regardless of what directory you're in. I've been using mh with this modification for weeks now.