-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add --no-cleanup option to iptables-wrapper-installer.sh #9
Conversation
Welcome @yoheiueda! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
iptables-wrapper-installer.sh
Outdated
shift | ||
done | ||
|
||
if [ -z "$no_sanity_check" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if [ -z "$no_sanity_check" ]; then | |
if [ -z "${no_sanity_check}" ]; then |
for consistency. (Bash scripts in k8s always do it that way... I think the idea is that you need the braces in some cases (like if there's text that comes directly after the variable expansion with no space between them) and it's better/more consistent to just always use the braces.)
(likewise with no_cleanup below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
iptables-wrapper-installer.sh
Outdated
if [ -z "$no_cleanup" ]; then | ||
mv "${iptables_wrapper_path}" "${sbin}/iptables-wrapper" | ||
else | ||
cp "${iptables_wrapper_path}" "${sbin}/iptables-wrapper" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also just always do "cp" rather than "mv" here, and then rm the original in the "# Cleanup" section below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This patch adds an option to suppress cleanup of iptables-wrapper binary and iptables-wrapper-installer.sh. We need this option when we build a container image by invoking iptables-wrapper-installer.sh from a read-only cache mount. Signed-off-by: Yohei Ueda <[email protected]>
/lgtm thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, yoheiueda The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Cache mounts of Docker builds are useful to share artifacts among multi-stage builds, but the cleanup mechanism of
iptables-wrapper-installer.sh
does not work well with cache mounts.This patch adds a new option
--no-cleanup
to suppress cleanup ofiptables-wrapper
binary andiptables-wrapper-installer.sh
.We can use this option to build a container image with a cache mount as follows.