Can I just ask you to take a quick look at the new PR: [WIP] - Netatmo add valves NRV by Terdious · Pull Request #2005 · GladysAssistant/Gladys · GitHub to confirm that it’s OK?
I’d like to be sure about the methodology when creating a PR from another branch and about the impact of the commit history, knowing that in this new PR we can see the history of the other PR. And that creating the new PR re-ran the tests on the previous one… always a bit worrying when you don’t do this often ^^
Hello @pierre-gilles !
Yes, so given the way I did it, that’s normal; for the moment I haven’t pushed anything, I did it locally.
That’s precisely why I’m asking. I started from my base branch netatmo-integration to create my new branch based on origin/gladys (to start from the code of that first PR and begin integrating the rest). I just pushed the new branch and then created the new PR linked to the GladysAssistant repo.
Is that the right method to keep things clean?
I will continue with the weather station modules one by one.
hoping that the first PR is good enough anyway ^^ but I’ve attached the branches so updates will be simplified. It shouldn’t impact the other PRs too much if changes are requested during the review!!
Edit:
@PhilippeMA, I’ll let you know when I’ve finished the complete station so you can test if it’s still OK!
Thanks for the feedback! For concurrency, I’d recommend staying at a maximum of 2 for things that run in the background like that. There are users on Raspberry Pis, we want to keep the software lightweight and avoid big spikes
Let me know as soon as the docs are up to date. Retest everything thoroughly to make sure you haven’t broken anything while making the fixes, and for me we can deploy this first version to production
New full tests on a fresh installation following the review fixes:
Documentation updated with current images (en and fr) :
Added a Dashboard view in the conclusion :
Docker image terdious/gladys:netatmo-integration-dev updated if needed :
Edit: The following 4 PRs are ready for review with the updates from the first. You can look at them when you have time, but FYI they’re really very minor — you’ll see. For now I’ve only included what’s already there (except for the first one — the weather station — which required 1 or 2 additional constants but nothing major).
Docker image terdious/gladys:netatmo-add-module-outdoor-weather-station for those who want it :
Rain gauge (simple) and Anemometer (a bit more difficult - dashboard front-end addition) in progress
In the end I went back a bit, I redid the branches and PRs from the up-to-date master. It was too much of a pain to update and it’s much cleaner.
So I have 2 PRs ready for review:
@pierre-gilles could you review the 6 PRs (I made them independent but the order is important, at least for PRs 1 and 2).
And tell me which you prefer, I have a branch that integrates all six of these devices. It can make small fixes applied incrementally easier and especially the review (46 files on the single branch versus 6 PRs × ~20 files when split since most files are affected in each PR).
@PhilippeMA, if you’re up for testing the whole weather station: docker pull terdious/gladys:netatmo-features-nrv-weather
Just remember to commit to the right branch
