Merge ~mitchdz/ubuntu/+source/apache2:noble-add-ubuntu-logo into ubuntu/+source/apache2:ubuntu/devel

Proposed by Mitchell Dzurick
Status: Merged
Merge reported by: Mitchell Dzurick
Merged at revision: 287e06983dde72b799fef4d6b13a94eaa4e28c94
Proposed branch: ~mitchdz/ubuntu/+source/apache2:noble-add-ubuntu-logo
Merge into: ubuntu/+source/apache2:ubuntu/devel
Diff against target: 68 lines (+39/-0)
3 files modified
debian/changelog (+7/-0)
debian/tests/check-ubuntu-branding (+28/-0)
debian/tests/control (+4/-0)
Reviewer Review Type Date Requested Status
Sergio Durigan Junior (community) Approve
Canonical Server Reporter Pending
git-ubuntu import Pending
Review via email:

Description of the change


ppa-dev-tools.ppa tests --show-url ppa:mitchdz/apache2-lp1947459-missing-ubuntu-logo --release noble
  - apache2/2.4.57-2ubuntu3~noble1
    + ✅ apache2 on noble for amd64 @ 14.11.23 20:26:33
      • Log:
    + ✅ apache2 on noble for arm64 @ 14.11.23 21:04:37
      • Log:
    + ✅ apache2 on noble for armhf @ 14.11.23 21:02:15
      • Log:
      • Status: FAIL
      • run-test-suite FAIL 🟥
      • duplicate-module-load PASS 🟩
      • default-mods PASS 🟩
      • htcacheclean PASS 🟩
      • ssl-passphrase PASS 🟩
      • check-http2 PASS 🟩
      • check-ubuntu-branding PASS 🟩
      • chroot FAIL 🟥
      • chroot FAIL 🟥
    + ✅ apache2 on noble for ppc64el @ 14.11.23 20:34:34
      • Log:
    + ✅ apache2 on noble for s390x @ 14.11.23 20:25:59
      • Log:

NOTE: armhf is passing all tests, possible bug in ppa-dev-tools filed here -

To post a comment you must log in.
Revision history for this message
Mitchell Dzurick (mitchdz) wrote :

Removed breaks-testbed as restriction as shouldn't be needed. Re-running ppa autopkgtest and will set back to Needs-Review once done.

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Thanks, Mitchell.

I'm leaving a few comments to improve the dep8 test below. Let me know when you address those and I'll finish the review & sponsorship.

review: Needs Fixing
Revision history for this message
Mitchell Dzurick (mitchdz) wrote :

Thanks Sergio. I applied most suggestions except `set -o lastpipe` since I am using sh instead of bash in this script.

local autopkgtest result on amd64:

$ autopkgtest -U -s -o "dep8-apache2" --test-name=check-ubuntu-branding ../apache2_2.4.57-2ubuntu3\~noble3.dsc -- qemu "/var/lib/adt-images/autopkgtest-noble-amd64.img"
autopkgtest [23:36:22]: test check-ubuntu-branding: [-----------------------
+ ubuntu_logo_path=icons/ubuntu-logo.png
+ grep -qF icons/ubuntu-logo.png
+ curl -s -k http://localhost
+ tr -d [:space:]
+ cut -d -f 2-
+ grep Content-Type
+ curl -s -I http://localhost/icons/ubuntu-logo.png
+ content_type=image/png
+ expected=image/png
+ [ image/png != image/png ]
autopkgtest [23:36:23]: test check-ubuntu-branding: -----------------------]
check-ubuntu-branding PASS
autopkgtest [23:36:23]: test check-ubuntu-branding: - - - - - - - - - - results - - - - - - - - - -
autopkgtest [23:36:24]: @@@@@@@@@@@@@@@@@@@@ summary
check-ubuntu-branding PASS

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Thanks, Mitchell.

It's not a problem to require bash to run the script, since it will be installed inside the autopkgtest testbed due to the package's priority.

I really think we should use pipefail here because of two things: first, you're not explicitly checking the return status of each command (due to "set -e", which is problematic by itself, but let's not go there), and second because of how you're calculating the value of ${content_type}:

content_type=$(curl -s -I http://localhost/$ubuntu_logo_path | grep Content-Type | cut -d ' ' -f 2- | tr -d '[:space:]')

Without pipefail, this will always succeed. Even though you're explicitly checking the content of the variable below, it's nevertheless a good practice to fail early here.

I hope this makes sense, but let me know if you still have reservations about using bash explicitly.

review: Needs Fixing
Revision history for this message
Mitchell Dzurick (mitchdz) wrote (last edit ):

I agree pipefail should be used here. same for `set -e`, I'm not the biggest fan of it, but kept it in for consistency across test scripts.

Since 2 tests do use bash (default-mods && run-test-suite) I'll change the interpreter to use bash and use pipefail to alleviate those concerns.

Revision history for this message
Mitchell Dzurick (mitchdz) wrote :

Sergio, I went to enable pipefail and went down a nice rabbit hole because the script became nondeterministic. I narrowed it down to this fun quirk -

I'm revising the script to inherently do better error handling.

Revision history for this message
Mitchell Dzurick (mitchdz) wrote :

reworked the script now. Ready for review again.

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Thanks, Mitchell.



$ dput ssh-ubuntu apache2_2.4.57-2ubuntu3_source.changes
D: Setting host argument.
Checking signature on .changes
gpg: /home/sergio/work/apache2/apache2_2.4.57-2ubuntu3_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/apache2/apache2_2.4.57-2ubuntu3.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ssh-ubuntu (via sftp to
  Uploading apache2_2.4.57-2ubuntu3.dsc: done.
  Uploading apache2_2.4.57-2ubuntu3.debian.tar.xz: done.
  Uploading apache2_2.4.57-2ubuntu3_source.buildinfo: done.
  Uploading apache2_2.4.57-2ubuntu3_source.changes: done.
Successfully uploaded packages.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index 118f242..dc4a0c2 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
1apache2 (2.4.57-2ubuntu3) noble; urgency=medium
3 * d/icons/ubuntu-logo.png: add Ubuntu image for welcome page (LP: #1947459).
4 * d/t/check-ubuntu-branding: add check for ubuntu branding.
6 -- Mitchell Dzurick <> Mon, 13 Nov 2023 10:49:48 -0700
1apache2 (2.4.57-2ubuntu2) mantic; urgency=medium8apache2 (2.4.57-2ubuntu2) mantic; urgency=medium
3 * d/control: Upgrade lua build dependency to 5.410 * d/control: Upgrade lua build dependency to 5.4
diff --git a/debian/icons/ubuntu-logo.png b/debian/icons/ubuntu-logo.png
4new file mode 10064411new file mode 100644
index 0000000..eee686c
5Binary files /dev/null and b/debian/icons/ubuntu-logo.png differ12Binary files /dev/null and b/debian/icons/ubuntu-logo.png differ
diff --git a/debian/tests/check-ubuntu-branding b/debian/tests/check-ubuntu-branding
6new file mode 10064413new file mode 100644
index 0000000..0bf90b6
--- /dev/null
+++ b/debian/tests/check-ubuntu-branding
@@ -0,0 +1,28 @@
3# Check the ubuntu branding exists
4set -uxe -o pipefail
8# Use curl to fetch the HTML content and check its exit status
9if html_content=$(curl -s http://localhost); then
10 # The curl command succeeded, so proceed with further processing
11 if ! [[ "$html_content" =~ "$ubuntu_logo_path" ]]; then
12 echo "ERROR: $ubuntu_logo_path string not found in html page"
13 exit 1
14 fi
16 # The curl command encountered an error
17 echo "ERROR: Curl command failed to fetch web content"
18 exit 1
21# Check the type of $ubuntu_logo_path
22content_type=$(curl -s -I http://localhost/$ubuntu_logo_path \
23 | grep Content-Type | cut -d ' ' -f 2- | tr -d '[:space:]')
25if [ "$content_type" != "$expected" ]; then
26 echo "Content-Type is not $expected it is $content_type"
27 exit 1
diff --git a/debian/tests/control b/debian/tests/control
index 2453137..8a93e5f 100644
--- a/debian/tests/control
+++ b/debian/tests/control
@@ -23,6 +23,10 @@ Tests: check-http2
23Restrictions: needs-root allow-stderr breaks-testbed23Restrictions: needs-root allow-stderr breaks-testbed
24Depends: apache2, curl, ssl-cert, nghttp2-client24Depends: apache2, curl, ssl-cert, nghttp2-client
26Tests: check-ubuntu-branding
27Restrictions: allow-stderr
28Depends: apache2, curl
26Tests: chroot30Tests: chroot
27Features: no-build-needed31Features: no-build-needed
28Restrictions: needs-root allow-stderr breaks-testbed32Restrictions: needs-root allow-stderr breaks-testbed


People subscribed via source and target branches