V3.0.0 - Integrate Solar Only Support #386
Conversation
| echo "" | ||
| while : | ||
| do | ||
| read -r -p "Select profile: " response |
There was a problem hiding this comment.
Minior thought: I wonder if we make the default 1 by also "Select profile [1]: " which means someone leaving it blank (enter) will get the default. We do that for other defaults.
There was a problem hiding this comment.
Also, the way the logic runs here, it will only prompt for configuration change (default or solar-only) the first time you run setup. I wonder if we should have a condition prompt like, "You are using the Powerwall (vs Solar-only) configuration, do you want to change that? [y/N]" to allow someone to switch. I suspect the switch would likely only be caused by an error running it first and selecting the wrong one, or if someone with Solar adds Powerwalls. What do you think?
There was a problem hiding this comment.
After a few more test and tweaking my idea, I'm leaning towards just leaving it as you have it. Keeping it simple for v3.0.0 makes the most sense. I'm going to test a bit more and then accept the PR if all looks well.
There was a problem hiding this comment.
As it happens, I had originally written the setup as you described, with a default on the "Select profile" prompt if the user pressed enter, and also allowing the chosen profile to be changed by re-running setup...
I later changed my thinking about this, and went with a forced prompt (user MUST make a selection, to prevent accidentally pressing enter and choosing the config profile without consciously looking at the options and making a selection). And also, NOT allowing the chosen profile to be changed later, as swapping and changing did not seem sensible (and could be achieved if you really wanted to by manually editing compose.env).
If we wanted to pivot to this method in future I had written some prelim methods to handle it as below:
# Read list of docker compose profiles from env file
. "${COMPOSE_ENV_FILE}"
IFS=',' read -a PROFILES <<< ${COMPOSE_PROFILES}
PROFILE="none"
current="1"
changed=0
index=-1
for i in "${!PROFILES[@]}"; do
if [[ "${PROFILES[$i]}" == "default" ]]; then
PROFILE="default"
current="1"
index=$i
break
elif [[ "${PROFILES[$i]}" == "solar-only" ]]; then
PROFILE="solar-only"
current="2"
index=$i
break
fi
done
echo "Select Powerwall-Dashboard Setup Profile"
echo ""
echo "Current Setup Profile: ${PROFILE}"
echo ""
echo "1 - default (Powerwall w/ Gateway on LAN)"
echo "2 - solar-only (No Gateway - data retrieved from Tesla Cloud)"
echo ""
while [ 1 ]; do
read -p "Select profile: [${current}] " response
if [ -z "${response}" ]; then
response="${current}"
fi
if [[ "${response}" == "1" ]]; then
if [[ "${PROFILE}" != "default" ]]; then
changed=1
fi
PROFILE="default"
elif [[ "${response}" == "2" ]]; then
if [[ "${PROFILE}" != "solar-only" ]]; then
changed=1
fi
PROFILE="solar-only"
else
continue
fi
break
done
# Update list of docker compose profiles and save to env file
if [ $changed -eq 1 ]; then
if [ $index -ge 0 ]; then
PROFILES[$index]="${PROFILE}"
else
PROFILES+=("${PROFILE}")
fi
sed -i.bak -E "/(^#|^)COMPOSE_PROFILES=/d" "${COMPOSE_ENV_FILE}"
echo "COMPOSE_PROFILES=$(IFS=,; echo "${PROFILES[*]}")" >> "${COMPOSE_ENV_FILE}"
fiThis was before the Docker Compose Helper Functions so would need some further changes if we were to use it. But that was the gist of what I had in mind originally.
However I do think the current implementation is best to keep it simple.
There was a problem hiding this comment.
On the default:
After I made that comment, in one of my test, I had hit enter twice after running the setup. So your point is 100% valid. Let's keep it mandatory to enforce the user attention.
On future setup runs:
I'm fine with how it is for now. However, the intent was that setup.sh could be run to fix a broken setup (user or other error). For a future enhancement, it may be good to have setup.sh check compose.env and report what was selected and offer resetting. Something like: "Existing configuration found: Your profile is set up for solar-only, do you wish to change that?" If they do, nuke the COMPOSE_PROFILES setting and let setup.sh continue with setup (prompting for 1/2).
| fi | ||
| CURRENT_USER="$(id -u):$(id -g)" | ||
| if [ "${CURRENT_USER}" != "${PWD_USER}" ]; then | ||
| echo "WARNING: Your current user uid/gid does not match the ${desc}." |
There was a problem hiding this comment.
I think this may help to solve a lot of potential issues, hopefully.
I tested on some different systems and ensured I was installing with a different uid/gid, and having this a prompt during setup to configure the system for that user solved those permission issues (or at the least will highlight the potential issues to the user).
There was a problem hiding this comment.
I would say that over half of the install related issues were related to this. This is gold! Thanks.
| # Windows Git Bash docker exec compatibility fix | ||
| if type winpty > /dev/null 2>&1; then | ||
| shopt -s expand_aliases | ||
| alias docker="winpty -Xallow-non-tty -Xplain docker" |
There was a problem hiding this comment.
I'm glad you discovered/added this!
There was a problem hiding this comment.
Results of my adventure down the Windows rabbit hole....
|
✅ Checking Upgrade on RPi Setup from 2.10.0 to 3.0.0 - Steps to force upgrade: git pull
gh pr checkout 386
cp upgrade.sh tmp.sh
bash tmp.sh upgrade
...
./verify.shOutput New Install |
|
🔴 Issue with a Ubuntu hosted instance during upgrade: |
|
Issue corrected with v3.0.1. |
|
Thanks for this upgrade @mcbirse !!! 🙏 Please feel free to propose an alternative to my v3.0.1 patch. I need to add the docker-compose V1 hosts to my testing plan going forward. In any case, it seems to work. Looking for any input from others. |
|
Hi @jasonacox - I committed directly a bug fix for the v3.0.1 patch as I believe the order in which the checks for docker compose V1/V2 need to be the other way round. i.e. check for V2 first using Why check for V2 first? The Running both commands on my Alpine Linux system produces the below: $ docker-compose version
Docker Compose version v2.17.3$ docker compose version
Docker Compose version v2.17.3So running the first command (with the dash) is not a definitive test to say "only Docker Compose V1 is installed", so we should check for V2 first. This is just a quick fix, hopefully there are no issues with it. I would still like to look further for other alternatives to the patch to support Docker Compose V1 users, however the current solution is good I believe. |
|
Looking into this issue further, Docker Compose V1 does actually support Profiles, as long as the version being used is I wonder if it would be better to check the version of Docker Compose before resorting to using the fix? Or, perhaps, we could run something similar to below to check for errors instead, which validates the compose config. if docker compose -f powerwall.yml $pwextend config > /dev/null 2>&1; then
#all good
else
#failed - assume no profiles support
fiI do also wonder how many people are still using Docker version older than |
|
Good call/fix on testing for docker V2 first.
Fair point! I happen to still have an old Ubuntu 18.04.6 system running and it has docker-compose 1.17.1. I have intended to upgrade it but also seemed to find it mirroring some of the odd behaviors Synology NAS users would report. This is the curse of any project. When do you stop supporting old version when it has spread to so many platforms? :) I ran this: if docker-compose -f powerwall.yml config > /dev/null 2>&1; then
echo "You have a current docker-compose"
else
echo "You have an old docker-compose"
fiIt works. "You have an old docker-compose". I suggest we do something like this: echo "Running Docker Compose..."
if docker compose version > /dev/null 2>&1; then
# Build Docker (v2)
docker compose -f powerwall.yml $pwextend $@
else
if docker-compose version > /dev/null 2>&1; then
# Build Docker (v1)
if docker-compose -f powerwall.yml config > /dev/null 2>&1; then
pwconfig="powerwall.yml"
else
echo "** WARNING **"
echo " You have an old version of docker-compose that will"
echo " be depreciated in a future release. Please upgrade or"
echo " report your use case to the project."
echo ""
echo "Applying workaround for old docker-compose..."
pwconfig="powerwall-v1.yml"
if get_profile "solar-only"; then
pwconfig="powerwall-v1-solar.yml"
fi
fi
docker-compose -f $pwconfig $pwextend $@
else
echo "ERROR: docker-compose/docker compose is not available or not running."
echo "This script requires docker-compose or docker compose."
echo "Please install and try again."
exit 1
fi
fiThis will still apply the workaround for now, but provide a user WARNING and hopefully prompt the community to report any edge use cases we need to consider before removing old docker-compose support. What do you think? Successful test output on my old system: And success with docker compose V2 system: |
|
I added these changes to a v3.0.2 branch - 5ed0838 |
Great solution, I like this! I committed a small change... since the Looks good to merge to me. |
|
Thanks, @mcbirse ! Merged. v3.0.2 is live. |
v3.0.0 - Updates
Integrate Solar Only Support
Related