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.

Sign up for GitHub

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

Jump to bottom

Fix shellcheck lint errors in cluster/addons/fluentd-elasticsearch/fl……uentd-es-image/run.sh #74187

Merged
merged 1 commit into from
Feb 22, 2019
Merged

Fix shellcheck lint errors in cluster/addons/fluentd-elasticsearch/fl……uentd-es-image/run.sh #74187

merged 1 commit into from
Feb 22, 2019

Conversation

xichengliudui
Copy link
Contributor

@xichengliudui xichengliudui commented Feb 18, 2019 •
edited

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?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 18, 2019
@xichengliudui
Copy link
Contributor Author

xichengliudui commented Feb 18, 2019 •
edited

/assign @coffeepac @BenTheElder

@k8s-ci-robot
Copy link
Contributor

@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.
For more information please see the contributor guide

In response to this:

/assign @coffeepac @bendrucker

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.

BenTheElder
BenTheElder reviewed Feb 18, 2019
View reviewed changes
cluster/addons/fluentd-elasticsearch/fluentd-es-image/run.sh Outdated
@@ -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"
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

@xichengliudui xichengliudui mentioned this pull request Feb 18, 2019
Closed
@xichengliudui xichengliudui force-pushed the fixgolint0218 branch 2 times, most recently from 446cf13 to 1dadd36 Compare February 19, 2019 02:54
@xichengliudui
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

1 similar comment
@xichengliudui
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@coffeepac
Copy link
Contributor

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2019
@xichengliudui
Copy link
Contributor Author

/assign @fejta

fejta
fejta reviewed Feb 20, 2019
View reviewed changes
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

cluster/addons/fluentd-elasticsearch/fluentd-es-image/run.sh
@@ -20,4 +20,5 @@
# For systems without journald
mkdir -p /var/log/journal

# shellcheck disable=SC2086
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that ok?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been completed

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2019
@spiffxp
Copy link
Member

spiffxp commented Feb 20, 2019

@fejta @BenTheElder you two are going in circles, which is the correct approach here

@coffeepac
Copy link
Contributor

my preference is to have the shellcheck enabled. I'll test the ${} version tonight to make sure it works.

fejta
fejta reviewed Feb 20, 2019
View reviewed changes
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

cluster/addons/fluentd-elasticsearch/fluentd-es-image/run.sh Outdated
@@ -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}"
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2019
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2019
@coffeepac
Copy link
Contributor

/no-approve

@coffeepac
Copy link
Contributor

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019
@coffeepac
Copy link
Contributor

quoting the parameters doesn't work. this is sh which lacks the necessary array functionality.

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.

@fejta
Copy link
Contributor

fejta commented Feb 21, 2019

Thanks Pat for the great detective work

@xichengliudui
Fix shellcheck lint errors in cluster/addons/fluentd-elasticsearch/fl…
053332a 
…uentd-es-image/run.sh

update pull request

update pull request

update pull request

update pull request

update pull request
@xichengliudui
Copy link
Contributor Author

/assign @fejta @coffeepac

@xichengliudui
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@coffeepac
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019
@xichengliudui
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2019
@xichengliudui
Copy link
Contributor Author

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Feb 22, 2019
@k8s-ci-robot k8s-ci-robot merged commit 125dc6c into kubernetes:master Feb 22, 2019
@coffeepac coffeepac mentioned this pull request May 31, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coffeepac coffeepac coffeepac left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@fejta fejta fejta left review comments

@gmarek gmarek Awaiting requested review from gmarek

@piosz piosz Awaiting requested review from piosz

Assignees

@coffeepac coffeepac

@fejta fejta

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@xichengliudui @k8s-ci-robot @coffeepac @spiffxp @fejta @BenTheElder