Code review comment for ~mitchdz/ubuntu/+source/apache2:noble-add-ubuntu-logo

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

« Back to merge proposal