Skip to content

Conversation

hemanthmalla
Copy link
Member

Modifiers are used to dynamically pass ARGs to Dockerfiles. While building images in CI, for example while building images with race detection enabled, modifiers are set to "LOCKDEBUG=1 RACE=1". Quotes don't get interpreted and are considered just like any other character, this leads to make interpreting "LOCKDEBUG to 1 and RACE to 1". This works currently because in most cases we simply check if the variable is non-empty. This is especially confusing if a new variable is added to modifiers and evaluated against actual value passed in.

Noticed this while trying to add a new variable in #35328

We do have shell set to bash everywhere, So I'm not sure why quotes aren't trimmed by the shell before passing it on to docker builder in CI.

cilium/Makefile.defs

Lines 4 to 5 in 98ada1a

SHELL := /usr/bin/env bash
.SHELLFLAGS := -eu -o pipefail -c

Modifiers are used to dynamically pass ARGs to Dockerfiles. While
building images in CI, for example while building images with race
detection enabled, modifiers are set to "LOCKDEBUG=1 RACE=1". Quotes
don't get interpreted and are considered just like any other character,
this leads to make interpreting "LOCKDEBUG to 1 and RACE to 1". This
works currently because in most cases we simply check if the variable is
non-empty. This is especially confusing if a new variable is added to
modifiers and evaluated against actual value passed in.

Noticed this while trying to add a new variable in
cilium#35328

Signed-off-by: Hemanth Malla <hemanth.malla@datadoghq.com>
@hemanthmalla hemanthmalla requested review from a team as code owners October 17, 2024 22:19
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 17, 2024
@github-actions github-actions bot added the cilium-cli This PR contains changes related with cilium-cli label Oct 17, 2024
@sayboras sayboras added the release-note/misc This PR makes changes that have no direct user impact. label Oct 18, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 18, 2024
Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Thank you!

@tklauser
Copy link
Member

/test

@tklauser tklauser enabled auto-merge October 22, 2024 08:03
@tklauser tklauser added this pull request to the merge queue Oct 25, 2024
Merged via the queue into cilium:main with commit 73f183d Oct 25, 2024
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cilium-cli This PR contains changes related with cilium-cli release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants