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

Allow setting an additional command to run on container start #1467

Closed
wants to merge 1 commit into from

Conversation

PromoFaux
Copy link
Member

What does this PR aim to accomplish?:

I'm not 100% sure on this one, but I'm not going to throw the idea away immediately without first some discussion around it.

My main use case is per the comments added in start.sh, namely:

# Example use case: If mounting the web repo from the host, we need to run `git config --global --add safe.directory /var/www/html/admin`
#                   in order for the updatechecker to work

By setting ADDITIONAL_COMMAND: "git config --global --add safe.directory /var/www/html/admin", I can then bypass the following message when mounting into /var/www/html/admin. This is not a common occurrence in userland, so seems silly to run that command regardless (though, potentially OK?)

image

Also allows for silly things like

image

I don't think this is a variable that needs documenting. Or maybe it does. Discuss.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

@PromoFaux PromoFaux requested a review from a team October 29, 2023 21:02
…arts. Undocumented variable. Potential for danger? Of course. But maybe it is useful. Commented use case - up for discussion

Signed-off-by: Adam Warner <[email protected]>
@yubiuser
Copy link
Member

I'm not a big fan of allowing arbitrary command injections during container startup on a deployed user-facing image.

I'm also not sure why you experienced the safe.directory issue. We got some issue reports on core about this a year ago since git started to check for the owner of the respective git repo. However, this should not be an issue here, as we run pihole updatechecker as root within the container and root should have enought priviledges to access .git

@PromoFaux
Copy link
Member Author

Yeah, it feels icky to be honest, was just throwing it against the wall.

If you mount an external git repo of the web repo into your container, do you see the same safe.directory issue?

@yubiuser
Copy link
Member

If you mount an external git repo of the web repo into your container, do you see the same safe.directory issue?

Yes.

[✓] Cleaning up stray matter
pihole  | 
pihole  |   [✓] Done.
pihole  | fatal: detected dubious ownership in repository at '/var/www/html/admin'
pihole  | To add an exception for this directory, call:
pihole  | 
pihole  | 	git config --global --add safe.directory /var/www/html/admin
pihole  | fatal: detected dubious ownership in repository at '/var/www/html/admin'
pihole  | To add an exception for this directory, call:
pihole  | 
pihole  | 	git config --global --add safe.directory /var/www/html/admin
pihole  |   [i] Docker start setup complete
pihole  | 
pihole  |   [i] pihole-FTL (no-daemon) will be started as pihole


@PromoFaux PromoFaux closed this Nov 3, 2023
@PromoFaux PromoFaux deleted the v6/additional_command branch September 2, 2024 22:52
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.

2 participants