Skip to content

tests/containers/fips-nginx: fix typos and add trap in build.sh#4492

Closed
HuijingHei wants to merge 1 commit intocoreos:mainfrom
HuijingHei:fix-typos
Closed

tests/containers/fips-nginx: fix typos and add trap in build.sh#4492
HuijingHei wants to merge 1 commit intocoreos:mainfrom
HuijingHei:fix-typos

Conversation

@HuijingHei
Copy link
Copy Markdown
Contributor

Fixes #4477

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request fixes a typo in the fips-nginx README and improves the build.sh script by adding a trap to ensure cleanup of the temporary directory. I've suggested a small improvement to make the trap more robust.

Comment thread tests/containers/fips-nginx/build.sh Outdated

tmpdir="$(mktemp -d)"
cp Containerfile ${tmpdir}
trap 'rm -rf "${tmpdir}"' EXIT
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

For better robustness, it's a good practice to change out of the directory that is about to be removed. While rm -rf on the current working directory often works, it's not guaranteed to be portable or successful in all cases (e.g., on certain filesystems or configurations). Changing to the home directory with cd before the rm command ensures the cleanup is more reliable.

Suggested change
trap 'rm -rf "${tmpdir}"' EXIT
trap 'cd && rm -rf "${tmpdir}"' EXIT

@HuijingHei
Copy link
Copy Markdown
Contributor Author

Next time I think better to let gemini review again before merge.

@HuijingHei
Copy link
Copy Markdown
Contributor Author

Close this as this is reverted in #4506

@HuijingHei HuijingHei closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant