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

Introduce additional docker tags for the debian version. #635

Merged

Conversation

lightswitch05
Copy link
Member

@lightswitch05 lightswitch05 commented Jun 7, 2020

Introduce additional docker tags for the Debian version, allows building both stretch and buster images.

Description

  • Added new docker tag variations to specify the Debian version (stretch, and buster).
  • Arch images are always as specific as possible: pihole/pihole:master-amd64-stretch. This is a bit of a breaking change, as there are no more pihole/pihole:master-amd64 tags... not sure if that is a problem?
  • Multiarch images have both the specific Debian version tags as well as the generic non-debian tags: pihole/pihole:master-stretch & pihole/pihole:master
  • Currently, the non-specific tags point to the 'stretch' images. Eventually it can be migrated to buster.
  • Use GitHub actions to do the builds. Although the script names include 'gh-actions' to differentiate them from the 'circle' scripts, there is zero logic that is specific to Github (ie. no Github environment variables).
  • armhf:buster & arm64:buster has an issue with ip route get. I think the issue is related to qemu, but I'm not sure. Update the validate_env function to only use ip route get if nc reports something strange.
  • Delete circle-ci job since it will fail with these changes. I've not included that here. I can delete the files if wanted.
  • This introduces buster and stretch tags, other then getting them to build and pass tests, I haven't done any further verification of the buster images

Motivation and Context

  • Opens a migration path for eventually having buster images being the primary Debian version and future debian versions
  • Uses Github Actions which are a little friendlier and easier to find. Doesn't require a circle-ci account.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@lightswitch05 lightswitch05 mentioned this pull request Jun 7, 2020
5 tasks
@lightswitch05
Copy link
Member Author

@lightswitch05 lightswitch05 commented Jun 19, 2020

I know there is a lot in here to review. Any initial thoughts? Is the lack of debian-generic architecture tags a blocker?

@dschaper
Copy link
Member

@dschaper dschaper commented Jun 19, 2020

I haven't looked it over since I try to stay out of @diginc way with docker but I'll give it a view if I'm not stepping on any toes.

@dschaper
Copy link
Member

@dschaper dschaper commented Jun 23, 2020

@diginc need any help?

@@ -46,7 +46,7 @@ validate_env() {
# Optional ServerIP is a valid IP
# nc won't throw any text based errors when it times out connecting to a valid IP, otherwise it complains about the DNS name being garbage
# if nc doesn't behave as we expect on a valid IP the routing table should be able to look it up and return a 0 retcode
if [[ "$(nc -4 -w1 -z "$ServerIP" 53 2>&1)" != "" ]] || ! ip route get "$ServerIP" > /dev/null ; then
if [[ "$(nc -4 -w1 -z "$ServerIP" 53 2>&1)" != "" ]] && ! ip route get "$ServerIP" > /dev/null ; then
Copy link
Member

@diginc diginc Jun 26, 2020

Choose a reason for hiding this comment

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

if nc doesn't behave as we expect on a valid IP the routing table should be able to look it up and return a 0 retcode

The comment states netcat can be unreliable so ip route is a fallback, so it should be ||, not && shouldn't it?

Copy link
Member Author

@lightswitch05 lightswitch05 Jun 26, 2020

Choose a reason for hiding this comment

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

no, if nc fails with an ||, then ip route get will never run. && means that both will run each time, and if both nc and ip route get fail, then we know for sure it is no good. If either of them pass, then we can be reasonably sure its valid.

Copy link
Member Author

@lightswitch05 lightswitch05 Jun 26, 2020

Choose a reason for hiding this comment

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

The better solution would be to figure out why armhf:buster & arm64:buster has an issue with ip route get. I spent some time on it, but when I starting running into things involving qemu, I gave up.

Copy link
Member Author

@lightswitch05 lightswitch05 Jun 26, 2020

Choose a reason for hiding this comment

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

The confusing part is the they are both negative tests, so != && !=

GIT_BRANCH=$(git rev-parse --abbrev-ref HEAD | sed "s/\//-/g")
GIT_TAG=$(git describe --tags --exact-match 2> /dev/null || true)

DEFAULT_DEBIAN_VERSION="stretch"
Copy link
Member

@diginc diginc Jun 26, 2020

Choose a reason for hiding this comment

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

we have a sprawling amount of default variables / updates to make in different places when those defaults change. We don't need it immediately but at some point, I'd like to centralize them with a very early export of defaults in a centrally included envs.sh that the pipeline uses before any builds/tests/uploads run. variables in the pipeline would work as a central location too but consider executing issues executing locally may have when putting things in the pipelines. The version number could migrate to this environment export too potentially.

diginc
diginc approved these changes Jun 29, 2020
* Added new docker tag variations to specify the debian version ('stretch', and 'buster').
* Arch images are alway as specific as possible: pihole/pihole:master-amd64-stretch
* Multiarch images have both the specific debian version tags as well as the generic non-debian tags: pihole/pihole:master-stretch & pihole/pihole:master
* Currently, the non-specific tags point to the 'stretch' images. Eventaully it can be migrated to 'buster'.
* Use GitHub actions to do the builds. Although the script names include 'gh-actions' to differentiate them from the 'circle' scripts, there is zero logic that is specific to Github (ie. no Github environment variables).
* 'armhf:buster' & 'arm64:buster' has an issue with `ip route get`. I think the issue is related to 'qemu', but I'm not sure. Update the `validate_env` function to only use `ip route get` if `nc` reports something strange.

Signed-off-by: Daniel <daniel@developerdan.com>
@lightswitch05 lightswitch05 force-pushed the feature/introduce-debian-version-tags branch from acfc8d1 to 752d83a Compare Jun 29, 2020
@lightswitch05
Copy link
Member Author

@lightswitch05 lightswitch05 commented Jun 29, 2020

I removed the references to my repo. Thanks for taking the time to review this massive PR!

@diginc diginc merged commit 88fd258 into pi-hole:dev Jun 29, 2020
8 checks passed
@pralor-bot
Copy link

@pralor-bot pralor-bot commented Sep 1, 2020

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/cannot-build-docker-image-with-newest-5-1-2-release-fully-locally/37245/3

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