-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Utilise FTL's native config upgrade functionality #1683
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving base on successful tests.
Please check the MM message about "Operation not permitted" errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this PR and noted two things.
We need to make sure, everyone still has mounted /etc/dnsmasq.d
mapped into the container at the first start for a successful migration. Currently or example compose file says, its not needed for most users.
https://github.com/pi-hole/docker-pi-hole/tree/development?tab=readme-ov-file#quick-start
Second, the assigned UID&GID
pihole | [i] PIHOLE_UID not set in environment, using default (100)
pihole | [i] PIHOLE_GID not set in environment, using default (101)
Normal user UID and GID usually start > 1000, but as the container runs as root they will be assigned 100 and 101, respectively.
Is this by design? Or should we change it?
Line 73 in 228608d
addgroup -S pihole && adduser -S pihole -G pihole && \ |
@yubiuser @PromoFaux What happens if they do not have |
Those configs are not migrated, otherwise it's fine docker-pi-hole/src/bash_functions.sh Lines 161 to 166 in 228608d
My comment was aiming on proper warning for the release notes or a note to the Readme |
If If a new container uses the same volumes used by the previous v5 container, the migration process will read the files and migrate them.
This is only needed when the user has custom files in this directory. Most users don't change these files. |
Okay, so it is really only about the comment on the README. They will also need it when they have static DHCP leases or CNAMEs. I don't think they have been stored elsewhere on v5. In v6, everything is in |
No, not by design - I guess it's just whatever the alpine image assigns when the user is created. We do have environment variables to set the u/gid's to whatever the user wishes. Maybe we just to make it clearer in the documentation that they can change it to match any ids on the host system if needed? |
I amended the Readme and rebased the branch |
… to defer the startup of FTL as we were previously doing. Signed-off-by: Adam Warner <me@adamwarner.co.uk>
…fit with the container output. Supress message in FTL migration output about environment variables, as these are not read in until FTLs first proper start Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: yubiuser <github@yubiuser.dev>
f3202b6
to
d13692b
Compare
Co-authored-by: Adam Warner <me@adamwarner.co.uk> Signed-off-by: yubiuser <github@yubiuser.dev>
|
What does this PR aim to accomplish?:
Taking inspiration from pi-hole/pi-hole#5830 - we use the new functionality within FTL to ensure V5 configs are properly migrated to V6.
Also taken a couple of tricks to improve the output of the docker container.
Example output from a new v6 container with 5 volumes:
Subsequent runs will not display the output messages as the configs will have already been migrated.
By submitting this pull request, I confirm the following:
git rebase
)