From 9e392f56b0b78f60bc2598c3cd99f8d7061cc550 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Thu, 20 Oct 2016 13:51:16 +1100 Subject: [PATCH] Don't set tracing in environment files Because environment files are sourced into the current environment, they shouldn't be setting global settings like tracing else they affect every preceeding import. This is quite confusing when only half your imports are traced in the logs, because it was either turned on, or off, by a preceeding environment import. There is a corresponding dib-run-parts change in I29f7df1514aeb988222d1094e8269eddb485c2a0 that will greatly increase debugability for environment files by deliberately logging what files are sourced and consistently turning on tracing around their import. This isn't strictly necessary (since dib-run-parts with the prior change will just turn tracing off after import anyway) but it's a decent cleanup for consistency. A bare-minimum dib-lint check is added. Documentation is updated. Change-Id: I10f68be0642835a04af7e5a2bc101502f61e5357 --- bin/dib-lint | 6 ++++++ doc/source/developer/developing_elements.rst | 11 ++++++----- elements/centos/environment.d/00-bootloader.bash | 8 -------- elements/manifests/environment.d/14-manifests | 8 -------- 4 files changed, 12 insertions(+), 21 deletions(-) diff --git a/bin/dib-lint b/bin/dib-lint index f75c1ccd6..c58865e5c 100755 --- a/bin/dib-lint +++ b/bin/dib-lint @@ -151,6 +151,12 @@ for i in $(find elements -type f \ fi fi + # check that environment files don't "set -x" + if [[ "$i" =~ (environment.d) ]]; then + if grep -q "set -x" $i; then + error "Environment file $i should not set tracing" + fi + fi # check that sudo calls in phases run outside the chroot look # "safe"; meaning that they seem to operate within the chroot diff --git a/doc/source/developer/developing_elements.rst b/doc/source/developer/developing_elements.rst index 0cee5946e..faadd5278 100644 --- a/doc/source/developer/developing_elements.rst +++ b/doc/source/developer/developing_elements.rst @@ -172,11 +172,12 @@ the image as executable files. Environment Variables ^^^^^^^^^^^^^^^^^^^^^ -To set environment variables for other hooks, add a file to your element -``environment.d``. - -This directory contains bash script snippets that are sourced before running -scripts in each phase. +To set environment variables for other hooks, add a file to your +element ``environment.d``. This directory contains bash script +snippets that are sourced before running scripts in each phase. Note +that because environment includes are sourced together, they should +not set global flags like ``set -x`` because they will affect all +preceeding imports. DIB exposes an internal ``$IMAGE_ELEMENT`` variable which provides elements access to the full set of elements that are included in the image build. This diff --git a/elements/centos/environment.d/00-bootloader.bash b/elements/centos/environment.d/00-bootloader.bash index 6478bc967..d406155d7 100755 --- a/elements/centos/environment.d/00-bootloader.bash +++ b/elements/centos/environment.d/00-bootloader.bash @@ -1,9 +1 @@ -#!/bin/bash - -if [ "${DIB_DEBUG_TRACE:-0}" -gt 0 ]; then - set -x -fi -set -eu -set -o pipefail - export DIB_EXTLINUX=1 diff --git a/elements/manifests/environment.d/14-manifests b/elements/manifests/environment.d/14-manifests index c2ae96dc9..a125a6f2f 100755 --- a/elements/manifests/environment.d/14-manifests +++ b/elements/manifests/environment.d/14-manifests @@ -1,5 +1,3 @@ -#!/bin/bash -# # Copyright 2014 Hewlett-Packard Development Company, L.P. # # Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -15,11 +13,5 @@ # under the License. # -if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then - set -x -fi -set -eu -set -o pipefail - export DIB_MANIFEST_IMAGE_DIR=${DIB_MANIFEST_IMAGE_DIR:-/etc/dib-manifests} export DIB_MANIFEST_SAVE_DIR=${DIB_MANIFEST_SAVE_DIR:-${IMAGE_NAME}.d/}