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

Add PADD to the image #1410

Merged
merged 4 commits into from
Aug 2, 2023
Merged

Add PADD to the image #1410

merged 4 commits into from
Aug 2, 2023

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Jul 31, 2023

Add the v6 development version of PADD to the image.

PADD expects the API at port 80, currently FTL still defaults to 8080. Therefore, PADD needs to be started with padd --port 8080

Screenshot at 2023-07-31 23-33-02

Signed-off-by: Christian König <[email protected]>
@yubiuser yubiuser requested a review from a team July 31, 2023 21:40
src/Dockerfile Outdated Show resolved Hide resolved
@yubiuser yubiuser force-pushed the padd/v6 branch 2 times, most recently from 101ca37 to 23572ef Compare August 1, 2023 11:52
src/Dockerfile Outdated
@@ -34,6 +34,10 @@ RUN apk add --no-cache \
ADD https://ftl.pi-hole.net/macvendor.db /macvendor.db
COPY crontab.txt /crontab.txt

# Add PADD to the container, too.
ADD --chmod=0755 https://raw.githubusercontent.com/pi-hole/PADD/PADD_FTLv6/padd.sh /usr/local/bin/padd
RUN alias padd='port=${FTLCONF_webserver_port%%,*} padd --port ${port:-8080} --secret ${FTLCONF_webserver_api_password}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alias is not much use here, it will need to be in start.sh like the following (tested)

# Add an alias for padd to the root user's bashrc
port="${FTLCONF_webserver_port%%,*}"
echo "alias padd='padd --port ${port:-8080} --secret ${FTLCONF_webserver_api_password}'" > /root/.bashrc

Another reason for it going into start.sh is that the FTLCONF_ variables will be defined by the user at runtime and we don't know them when we build the image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I FP'd it to the right place.

Copy link
Contributor

@edgd1er edgd1er Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry my suggestion was incomplete. I put $ to prevent variable substitution at build time.
the complete line to put in dockerfile is:
echo -e "alias padd='port=\${FTLCONF_webserver_port%%,*} padd --port \${port:-8080} --secret \${FTLCONF_webserver_api_password}'" >>/root/.bashrc

without the redirection to bashrc and placed in start.sh will do also, but bashrc will be rewritten at each container start, erasing possible user aliases and/or bash user defined settings.

Signed-off-by: Christian König <[email protected]>
src/start.sh Outdated
@@ -87,6 +87,10 @@ if [ "${INSTALL_DEV_TOOLS:-0}" -gt 0 ] ; then
apk add --no-cache nano less
fi

# Add an alias for padd to the root user's bashrc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placed in start.sh will do also, but bashrc will be rewritten at each container start, erasing possible user aliases and/or bash user defined settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be in start.sh because the user-defined FTLCONF_ environment variables will not be defined at build-time, so it need to be applied at run time.

@yubiuser another thought, to ensure we're not munging any user-defined .bashrc (though, one would argue there isn't much of a need to do so in a container such as this) Perhaps we cna do something like:

Dockerfile:

ADD --chmod=0755 https://raw.githubusercontent.com/pi-hole/PADD/PADD_FTLv6/padd.sh /usr/bin/padd

start.sh

port="${FTLCONF_webserver_port%%,*}"
echo "/usr/bin/padd --port ${port:-8080} --secret ${FTLCONF_webserver_api_password}'" > /usr/local/bin/padd
chmod +x /usr/local/bin/padd

Copy link
Contributor

@edgd1er edgd1er Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be in start.sh because the user-defined FTLCONF_ environment variables will not be defined at build-time, so it need to be applied at run time.

well, it doesn't have to be in start.sh, as bashrc is sourced when session starts, and using $ prevent bash to substitute variable at buildtime. that being said, defining a dynamic alias in start.sh will eventually add the same feature as an alias in bashrc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using $ prevent bash to substitute variable at buildtime.

Ah OK thanks, when I was tinkering about before it wasn't working - but that could have been for all manner of reasons, to be honest!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just noticed that the slash before the $ was removed in the reply when publishing the comment, there is may be a misunderstanding : here is a picture of the dockerfile of how it should look:
image

and the result in the image:
image

@PromoFaux
Copy link
Member

With the addition of pi-hole/PADD#362, the user need only pass in --secret to padd now in order for it to work.

Internally discussed, but as there is no guarantee that the password will be set via FTLCONF_webserver_api_password, (i.e some users prefer not to keep passwords in their compose files etc) it does not seem like a good idea to have an alias for padd inside the image. We can always add a hint in the readme on how users can add an alias themselves if they really need it

@yubiuser -can we have it just in the dockerfile please, no alias - and also if possible please make the padd branch a variable that can be passwed in at build time. (like the other repo branch arguments)

@yubiuser yubiuser requested a review from PromoFaux August 2, 2023 20:38
src/start.sh Outdated Show resolved Hide resolved
Signed-off-by: Adam Warner <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Aug 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@PromoFaux PromoFaux merged commit 661bc42 into development-v6 Aug 2, 2023
8 checks passed
@PromoFaux PromoFaux deleted the padd/v6 branch August 2, 2023 22:03
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.

3 participants