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

Fix lint script to run on macOS #73

Merged
merged 1 commit into from
Mar 1, 2021
Merged

Fix lint script to run on macOS #73

merged 1 commit into from
Mar 1, 2021

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Feb 22, 2021

Fix lint script to run on macOS 11.1

See

Before:

$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin20)
Copyright (C) 2007 Free Software Foundation, Inc.

$ ./build/scripts/find-lint.sh
./build/scripts/find-lint.sh: line 22: conditional binary operator expected

$ COMPLEMENT_LINT_CONCURRENCY=1 ./build/scripts/find-lint.sh
./build/scripts/find-lint.sh: line 22: conditional binary operator expected

After:

$ ./build/scripts/find-lint.sh
No issues found :)

$ COMPLEMENT_LINT_CONCURRENCY=1 ./build/scripts/find-lint.sh
No issues found :)

Alternative if we want COMPLEMENT_LINT_CONCURRENCY to be required:

...
if [[ ${COMPLEMENT_LINT_CONCURRENCY} ]]; then

After:

$ ./build/scripts/find-lint.sh
./build/scripts/find-lint.sh: line 22: conditional binary operator expected

$ COMPLEMENT_LINT_CONCURRENCY=1 ./build/scripts/find-lint.sh
No issues found :)

@@ -19,7 +19,7 @@ if [ ${1:-""} = "fast" ]
then args="--fast"
fi

if [[ -v COMPLEMENT_LINT_CONCURRENCY ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -v thing seems to only work on bash 4.2 and above, https://unix.stackexchange.com/a/212192/52320

On macOS 11.1

bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin20)
Copyright (C) 2007 Free Software Foundation, Inc.

See

 - `-v` only supported in newer bash 4.2 or above: https://unix.stackexchange.com/a/212192/52320
 - How to test for environemmnt variable existence, https://stackoverflow.com/a/13864829/796832
 - No-op: https://unix.stackexchange.com/a/287867/52320
@@ -19,7 +19,10 @@ if [ ${1:-""} = "fast" ]
then args="--fast"
fi

if [[ -v COMPLEMENT_LINT_CONCURRENCY ]]; then
if [ -z ${COMPLEMENT_LINT_CONCURRENCY+x} ]; then
Copy link
Contributor Author

@MadLittleMods MadLittleMods Feb 22, 2021

Choose a reason for hiding this comment

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

Instead of using the empty if-case, I tried using -n but it doesn't seem to behave the same as the -z and else

-n
string is not null.

-z
string is null, that is, has zero length

https://unix.stackexchange.com/a/109631/52320

if [ -n ${COMPLEMENT_LINT_CONCURRENCY+x} ]; then
$ ./build/scripts/find-lint.sh
./build/scripts/find-lint.sh: line 23: COMPLEMENT_LINT_CONCURRENCY: unbound variable

$ COMPLEMENT_LINT_CONCURRENCY=1 ./build/scripts/find-lint.sh
No issues found :)

MadLittleMods added a commit that referenced this pull request Feb 22, 2021
kegsay added a commit that referenced this pull request Feb 23, 2021
* Add application service support to blueprints

Split out from #68

* Revert always showing logs

* Add comment doc

* Some nits and remove the volume paths

 - Seems like the `Volumes` syntax is to create an anonymous volume, https://stackoverflow.com/a/58916037/796832
 - And lots of people not knowing what `Volumes` syntax is or what to do. Seems like Mounts is the thing to use
    - fsouza/go-dockerclient#155
    - https://stackoverflow.com/questions/55718603/golang-docker-library-mounting-host-directory-volumes
    - https://stackoverflow.com/questions/48470194/defining-a-mount-point-for-volumes-in-golang-docker-sdk

* Address review and add comment docs

* Revert lint change already in other PR #73

* Path escape AS IDs to avoid directory traversal attacks

Co-authored-by: Kegan Dougal <[email protected]>
@kegsay kegsay merged commit 2926435 into master Mar 1, 2021
@MadLittleMods
Copy link
Contributor Author

Thanks for the review and merge @kegsay 🐭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants