Skip to content

Conversation

jwendell
Copy link
Member

No description provided.

@jwendell jwendell requested a review from a team as a code owner September 23, 2025 18:07
fi
exec gosu envoybuild "$@"

gosu envoybuild "$@"
Copy link
Member

Choose a reason for hiding this comment

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

i think this should be exec

Copy link
Member Author

@jwendell jwendell Sep 23, 2025

Choose a reason for hiding this comment

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

Right, that was my only question on this change. I'm worried the thing after exec does not get executed. ShellCheck warns about it.

We'd need either remove the cleanup section, or suppress the warning, but I think the warning is legit.

Copy link
Member

@phlax phlax Sep 23, 2025

Choose a reason for hiding this comment

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

fair point - probs best to move it to something like

cleanup() {
    if [[ -f /cleanup.sh ]]; then
        . /cleanup.sh
    fi
}
trap cleanup EXIT

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't work. exec replaces the current process entirely, including any cleanup trap.

#!/bin/bash

set -eo pipefail

cleanup() {
  echo "b.sh has returned. Exiting a.sh."
}
trap cleanup EXIT

echo "This is a.sh. Invoking b.sh next..."
exec ./b.sh

^^ cleanup() is never called.

Copy link
Member

Choose a reason for hiding this comment

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

argh - thats annoying - something like this could probably work

gosu envoybuild "$@" &
pid=$!

wait $pid
status=$?

if [ -f /cleanup.sh ]; then
    . /cleanup.sh
fi

exit $status

but tbh - im happy if we just remove the cleanup for now - i dont think i was using it anywhere given it wasnt working

Copy link
Member Author

Choose a reason for hiding this comment

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

What's your intention with exec? AFAICT gosu already performs an exec:

Once the user/group is processed, we switch to that user, then we exec the specified process and gosu itself is no longer resident or involved in the process lifecycle at all.

Taken from its README.

Copy link
Member

Choose a reason for hiding this comment

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

keeping the proc as pid 1

Copy link
Member

Choose a reason for hiding this comment

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

and ensuring the signals are properly handled - pretty sure you should exec gosu in an entrypoint

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I'll remove the cleanup for now.

runs-on: ubuntu-latest

steps:
- name: Checkout repository
Copy link
Member

@phlax phlax Sep 23, 2025

Choose a reason for hiding this comment

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

can we do least indent for arrays in yaml files please

sort of related .. we have envoy.code.check - which can run is python and can run in ci etc - it has both yamllint and shellcheck

i reckon lets land this (with cleaned up yaml) and i can switch to the envoy one after

Signed-off-by: Jonh Wendell <[email protected]>
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.

2 participants