-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Fix shellcheck lint errors in cluster/addons/fluentd-elasticsearch/fl……uentd-es-image/run.sh #74187
Conversation
/assign @coffeepac @BenTheElder |
@xichengliudui: GitHub didn't allow me to assign the following users: bendrucker. Note that only kubernetes members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -20,4 +20,4 @@ | |||
# For systems without journald | |||
mkdir -p /var/log/journal | |||
|
|||
exec /usr/local/bin/fluentd $FLUENTD_ARGS | |||
exec /usr/local/bin/fluentd "$FLUENTD_ARGS" |
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.
this string probably does need to be split
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.
"${FLUENTD_ARGS}" is that ok?
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.
that will produce the same result, i'm pretty sure we should just disable the lint for this line since these are arguments and this is actually (hacky) desired behavior
446cf13
to
1dadd36
Compare
/test pull-kubernetes-e2e-gce |
1 similar comment
/test pull-kubernetes-e2e-gce |
/approve |
/assign @fejta |
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.
/lgtm
/hold
@@ -20,4 +20,5 @@ | |||
# For systems without journald | |||
mkdir -p /var/log/journal | |||
|
|||
# shellcheck disable=SC2086 |
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.
Please add a more detailed comment
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.
Is that ok?
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 they produce the same result please change it to "${FLUENTD_ARGS}" and remove the lint exception.
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.
Has been completed
1dadd36
to
b4164d8
Compare
b4164d8
to
58a31cb
Compare
@fejta @BenTheElder you two are going in circles, which is the correct approach here |
my preference is to have the shellcheck enabled. I'll test the |
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.
/lgtm
/approve
/hold
@@ -20,4 +20,4 @@ | |||
# For systems without journald | |||
mkdir -p /var/log/journal | |||
|
|||
exec /usr/local/bin/fluentd $FLUENTD_ARGS | |||
exec /usr/local/bin/fluentd "${FLUENTD_ARGS}" |
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.
Is there a reason we need to use exec?
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.
I think this should be correct, because after we modify FLUENTD_ARGS, the verify-shellcheck.sh check pass.
Before modification:
In ./cluster/addons/fluentd-elasticsearch/fluentd-es-image/run.sh line 23:
exec /usr/local/bin/fluentd $FLUENTD_ARGS
^-----------^ SC2086: Double quote to prevent globbing and word splitting.
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.
fluentd will gracefully stop (flush buffers, etc) if it receives the signal directly. I assume (not sure! would love an answer or a bit of a doc/code pointer!) that the kubelet sends a docker stop
or CRI equivalent to the pod's containers when the pod is being shut down.
if exec is not used, fluentd isn't PID 1 and doesn't get the signal.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coffeepac, fejta, xichengliudui 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 |
/no-approve |
/lgtm cancel |
quoting the parameters doesn't work. this is the wiki for this rule suggests this use case is an acceptable if unhappy exception. This needs to be a non-quoted string and should have the comment to prevent shellcheck failures on this line. |
Thanks Pat for the great detective work |
…uentd-es-image/run.sh update pull request update pull request update pull request update pull request update pull request
58a31cb
to
053332a
Compare
/assign @fejta @coffeepac |
/test pull-kubernetes-e2e-gce |
/lgtm |
/hold cancel |
/kind cleanup |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Ref #72956
Special notes for your reviewer:
Does this PR introduce a user-facing change?: