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

MAISTRA-2155 Fix shellcheck errors in Must Gather #13

Merged
merged 6 commits into from Mar 3, 2021

Conversation

luksa
Copy link
Contributor

@luksa luksa commented Mar 2, 2021

No description provided.

gather_istio Outdated Show resolved Hide resolved
Copy link
Member

@jwendell jwendell left a comment

Choose a reason for hiding this comment

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

Can you add a prow pre-submit test that runs shellcheck against this file?

In this repo, you'd need to add a lint target in Makefile that runs shellcheck like this: https://github.com/maistra/test-infra/blob/main/Makefile#L39

Then, open a PR in test-infra adding the job.

gather_istio Outdated
@@ -133,11 +140,11 @@ function getEnvoyConfigForPodsInNamespace() {
local logPath=${BASE_COLLECTION_PATH}/namespaces/${podNamespace}/pods/${podName}
mkdir -p ${logPath}

echo "Pilot config for pod ${podName}.${podNamespace} from istiod ${pilotName}.${controlPlaneNamespace}" 2>&1 1>${logPath}/pilotConfiguration
oc exec ${pilotName} -n ${controlPlaneNamespace} -c discovery -- bash -c "/usr/local/bin/pilot-discovery request GET /debug/config_dump?proxyID=${podName}.${podNamespace}" 2>&1 1>${logPath}/pilotConfiguration
echo "Pilot config for pod ${podName}.${podNamespace} from istiod ${pilotName}.${controlPlaneNamespace}" > ${logPath}/pilotConfiguration 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

nit: Strictly speaking 2>&1 is not necessary, I believe echo will never fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lol. At least if it does, there are likely bigger issues.

Copy link
Member

@jwendell jwendell left a comment

Choose a reason for hiding this comment

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

Don't forget to sync this change on product side - copy the script without the .sh extension or adjust the Dockerfile there

@maistra-bot maistra-bot merged commit 8f1d8da into maistra-2.0 Mar 3, 2021
@maistra-bot
Copy link
Contributor

In response to a cherrypick label: #13 failed to apply on top of branch "maistra-2.1":

Applying: Correct use of arrays (SC2178)
Applying: Avoid masking return values (SC2155)
Using index info to reconstruct a base tree...
M	gather_istio
Falling back to patching base and 3-way merge...
Auto-merging gather_istio
CONFLICT (content): Merge conflict in gather_istio
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Avoid masking return values (SC2155)
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@luksa luksa deleted the MAISTRA-2155 branch March 3, 2021 13:08
luksa added a commit to luksa/istio-must-gather that referenced this pull request Mar 8, 2021
* Correct use of arrays (SC2178)

* Avoid masking return values (SC2155)

* Prevent whitespace problems (SC2048)

* Ensure errors are written to the file instead of stdout (SC2069)

* Double quote to prevent globbing and word splitting (SC2086)

* Run ShellCheck on make lint
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.

None yet

4 participants