Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove erroneous bash evaluation from scripts/cli.sh #5121

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

macladson
Copy link
Member

Issue Addressed

#5041 introduced some hints for how to solve CI errors relating to the CLI help text files added in #4571

This had an unfortunate side effect. Since make cli and make cli-local were encased in back-ticks and echoed to the user, it caused Bash to evaluate them in the terminal.

We saw a case of this occurring on CI here.
The log line:

docker: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?.

indicates that the CI job attempted to spin up a nested docker container which failed.

There were also effects seen locally. Running make cli-local would actually end up triggering make cli in the event that changes were required so you would end up seeing Docker logs. Luckily, we did not echo the offending line in the event that no changes were made, which prevented a infinite recursion situation.

Proposed Changes

Replace the back-ticks with single quotes which do not get evaluated.

@macladson macladson added bug Something isn't working ready-for-review The code is ready for review infra-ci labels Jan 24, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Nice find! This would have been very strange to debug!

@michaelsproul
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Jan 25, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 0b6c898

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 25, 2024
mergify bot added a commit that referenced this pull request Jan 25, 2024
@mergify mergify bot merged commit 0b6c898 into sigp:unstable Jan 25, 2024
29 checks passed
@macladson macladson deleted the fix-cli-script branch January 25, 2024 05:01
danielramirezch pushed a commit to danielramirezch/lighthouse that referenced this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working infra-ci ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants