Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker Compose version v2.25.0 reports version is obsolete #454

Merged
merged 5 commits into from
Mar 23, 2024

Conversation

BJReplay
Copy link
Contributor

Addresses #453

Remove version tags - Docker Compose version v2.25.0 reports version is obsolete
Remove version from sample powerwall.extend.yml

Docker Compose version v2.25.0 reports version is obsolete
Updated instructions for Docker Compose V2
@jasonacox
Copy link
Owner

Thanks @BJReplay - I like the direction but see a problem with V1 docker-compose. Here is what I get on my V1 host:

Running Docker Compose...
ERROR: Invalid interpolation format for "weather411" option in service "services": "${PWD_USER:-1000:1000}"

It seems that version 3.5 is needed to support that. Unless you have another idea for us to try, we could look at removing V1 support but would want to provide warning before doing the upgrade. I don't know how widely V1 is used on older devices or NAS devices like Synology.

@BJReplay
Copy link
Contributor Author

Here is what I get on my V1 host:

Fair enough. I'm guessing you have a V1 host for testing compatibility.

It is just a warning, so it is OK to ignore, so perhaps the right approach is to just ignore it.

That said, I'd hate to be running something like a Synology (or anything else) that is stuck running 2016 era software; I'm running a 2015 vintage QNAP, but I would scrap it if it didn't have more up to date software available as I would see 2016 vintage software as such a massive security issue. I imagine I could find a number of serious security issues with docker between 2016 and now, for example.

@jasonacox
Copy link
Owner

Yes, I keep the old V1 system around for testing, but I think this is enough to help justify removing V1 support:

https://docs.docker.com/compose/compose-file/compose-versioning/

image

We can add logic to upgrade.sh to test to see what version the users is running and stop the upgrade until they upgrade their Docker compose version.

@BJReplay
Copy link
Contributor Author

Actually, hang on a minute; on your V1 host, what exactly are you running? As in version of docker and version of docker compose?

Because the docker file format that you're running is V2 (that's the version that introduced the services tag).

And V2 docker compose was introduced a long time ago; now it wasn't mandatory to upgrade to the new version of docker compose, but it does sound like you've got a version that has been unsupported.

I see (your reply popped in while writing this).

@BJReplay
Copy link
Contributor Author

So, to expand for a second: you can't have a services tag without the file being a V2 format. And, in that format, the version tag is optional. But only very early (pre 2016) version of docker-compose don't handle that. There's a small set of docker-compose versions that are new enough to handle the service tag but old enough to not handle the missing version tag.

I suspect it's a very small set.

It could be just our luck that's the set baked into Synology (or pick any other NAS) firmware, but it's unsupported.

@jasonacox
Copy link
Owner

Yes, my test box has docker-compose version 1.17.1-2. Based on what you say, we probably already broke older V1's by introducing the version 3.5 format. If it is a small set, more reason to remove support for V1. It also means, I can remove one of my tests. :)

I would still like to provide the check and ERROR message to the user to upgrade Docker before running upgrade.sh. I would like to avoid breaking a working system even if it is depreciated.

@BJReplay
Copy link
Contributor Author

I would still like to provide the check and ERROR message to the user to upgrade Docker before running upgrade.sh.

I'll have a look and see if I can work out what is required, and if I can, include that in this PR. If I think it's beyond my meagre bash skills, I'll let you know. I won't be able to test a V1 system, but I can test on my system to test the happy case (user already on V2) assuming I can get it working. Let me get back to you.

Check for Docker Compose V2
@BJReplay
Copy link
Contributor Author

Added a check to upgrade.sh. Basically borrowed the check from compose-dash.sh and modified it.

@jasonacox
Copy link
Owner

Looks good. I want to run some tests. Also @mcbirse , I would love your thoughts on this as well. Any concerns?

Thanks @BJReplay

@mcbirse
Copy link
Collaborator

mcbirse commented Mar 21, 2024

@jasonacox - I have not tested, but no concerns from my end.

I noted long ago the "version" tag was optional in the docker compose yaml V2 spec, although no warnings were being given at that stage...

I am definitely in favour of removing support for V1 and advising users to upgrade.

Apologies for not being active lately. Life/work has been hectic since trying to catch up from my covid knockdown/outage recently. 😠

@BJReplay
Copy link
Contributor Author

@mcbirse I hope you're feeling better: my second covid infection set me back as it exacerbated heart conditions that were previously well managed; it took me a long time to get back to feeling myself, so I wish you well.

@jasonacox jasonacox merged commit fcae91a into jasonacox:main Mar 23, 2024
@jasonacox
Copy link
Owner

jasonacox commented Mar 23, 2024

Test (PASS - correct behavior) on old V1 system:

$ ./upgrade.sh
Upgrade Powerwall-Dashboard from 4.1.2 to 4.2.0
---------------------------------------------------------------------
This script will attempt to upgrade you to the latest version without
removing existing data. A backup is still recommended.

Checking Docker Compose...
ERROR: Docker Compose V1 Found: Upgrade Required
See Migration Instructions at https://docs.docker.com/compose/migrate/

@jasonacox
Copy link
Owner

Thanks @BJReplay !

I set this as 4.2.0 (indicating possible breaking change for some folks due to the V1 support removal).

@BuongiornoTexas
Copy link
Contributor

@jasonacox - does this allow reverting of #442? From the discussion, the main reason for rejecting was to maintain compatability with docker-compose 1.17.

@jasonacox
Copy link
Owner

Honestly, I'm not smart enough to know how to do that. What would you like to see? Happy to push forward.

@BuongiornoTexas
Copy link
Contributor

I should rephrase - would you like to reinstitute the change? There wasn't much to it, and I can probably create a new PR with a bit of diff work on verify.sh.

@jasonacox
Copy link
Owner

OH! That was the logic in upgrade.sh that offered to upgrade older grafana.env, right? I made it more hard core (just nuked the old grafana.env with the new list you built, unless the user said otherwise). Was there another benefit that would help with customization setups like yours?

In any case, I'm happy to add it back in if you think it is helpful or leave it as is. And very happy with your tuned plugin list even brute-force mode. That was a good PR improvement. Start up time are significantly faster.

@BJReplay
Copy link
Contributor Author

Thanks @BJReplay !

I set this as 4.2.0 (indicating possible breaking change for some folks due to the V1 support removal).

Always a pleasure to contribute, even in a tiny way... Speaking of tiny; I'm just getting into home assistant and Tuya (I have just two "smart" devices in my house - most of the house has MR12 LED downlights that were upgrades from halogen) and the two devices were not Tuya devices, but Arlec (an Australian brand), but anyway, tuya internals. And, in my travels today I discovered, of course, that you wrote tinytuya! I've just got my light globes hooked up to local control via localtuya in HA, and I used tinytuya to understand how to set them up; they have a switch, brightness, and a scene setting but it wasn't obvious how to map the DPs. So thanks for that, as well!

@BJReplay BJReplay deleted the BJReplay-patch-1 branch March 23, 2024 06:49
@BuongiornoTexas
Copy link
Contributor

OH! That was the logic in upgrade.sh that offered to upgrade older grafana.env, right? I made it more hard core (just nuked the old grafana.env with the new list you built, unless the user said otherwise). Was there another benefit that would help with customization setups like yours?

In any case, I'm happy to add it back in if you think it is helpful or leave it as is.

My turn to discover that I should have checked what you did more carefully - very happy with the nuke and replace option. My version was around managing the few who have custom plugins (ironically, I'm not one of them).

I think in the interim, I'll just put the code in an archive and come back to it if it becomes relevant in the future. Less work for both of us and we are both happy with the current setup!

The one thing that might be worth considering is nuking all downloaded plugins with each upgrade. Grafana already downloads all plugins speciifed in the env file with each restart, but as far as I can tell it doesn't clean up old plugins - which was where some of my sluggishness in grafana was coming from.

@jasonacox
Copy link
Owner

you wrote tinytuya! ... So thanks for that, as well!

Thanks @BJReplay!! TinyTuya started off as a way for me to monitor power usage with Tuya smart plugs (you can see how it is related to this one). But I have to give credit to the incredible community of enthusiast and contributors there. It wouldn't be where it is without the brilliant folks there, especially @uzlonewolf who is an absolute wizard. All the recent protocol support came from his genius.

Always a pleasure to contribute, even in a tiny way...

Tiny is good! Thanks again for this contribution. Considering the age of Docker V1, I'm hopeful that we don't see a lot of impact (e.g. issues opened) but will watch for that. My fear on any of these updates is that we have forgotten to consider a part of the community that has limited ability to upgrade (thinking more of appliance style hosts like Synology). I know I should get one to add to my test suite, but life seems to find other plans for that money and time. 😂

Speaking of time... we are preparing to merge a fairly major rewrite of pypowerwall (see jasonacox/pypowerwall#77) thanks to @emptywee. It will help us make it much easier to accommodate any future Telsa API changes and modes (e.g. tedapi and FleetAPI support). I will be dropping a new pypowerwall container to test soon. If you have time to help test and provide feedback, that would be awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants