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

cmd/snap-confine/tests: fix shellcheck on recently added files #3258

Merged
merged 2 commits into from May 3, 2017

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented May 2, 2017

Shellcheck is not tested automatically becauseof packaging issues related to
that. This has lead to an issue where we merged something that fails (harmless)
shellcheck verification and is a problem for downstream packaging.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

@@ -0,0 +1,83 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

This file looks like it doesn't belong into this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, how the hell :D

Copy link
Contributor

@ogra1 ogra1 left a comment

Choose a reason for hiding this comment

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

one nitpick (not a blocker though)

@@ -60,7 +60,7 @@ SHM="$(mktemp -d -p /run/shm)"
trap 'rm -rf $TMP $SHM' EXIT

# name snap-confine as the test name for improved logging
L="$TMP/`basename $0`"
L="$TMP/$(basename "$0")"
Copy link
Contributor

Choose a reason for hiding this comment

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

only do:
L=$TMP/$(basename "$0")

there shoudl be no need for the outer quotes that makes it more confusing to read...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think shellcheck disagrees.

Shellcheck is not tested automatically becauseof packaging issues related to
that. This has lead to an issue where we merged something that fails (harmless)
shellcheck verification and is a problem for downstream packaging.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@morphis
Copy link
Contributor

morphis commented May 2, 2017

LGTM

@morphis
Copy link
Contributor

morphis commented May 2, 2017

Verified this fixes the build issues on Fedora.

@zyga zyga merged commit 8cc88b8 into snapcore:master May 3, 2017
@zyga zyga deleted the fix/shellcheck branch May 3, 2017 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants