nickjj / docker-django-example

A production ready example Django app that's using Docker and Docker Compose.
MIT License
1.22k stars 266 forks source link

running `./run flake8` on fresh clone gives `unbound variable` error #1

Closed chanana closed 3 years ago

chanana commented 3 years ago

Cloned the repository, ran the recommended setup instructions and then I got this:

./run flake8
./run: line 49: @: unbound variable
nickjj commented 3 years ago

Hi,

Which version of macOS are you using? I've noticed on certain macOS machines this happens but not all. I've also never seen it happen on native Linux or WSL.

A workaround is to remove the u in set -euo pipefail on line 3. I'm thinking about pushing that up as a change too. Having set -u protects you from having a script with unbound variables (common reasons for this are typos in variable names) but I guess some versions of Bash don't like it with $@.

chanana commented 3 years ago

Aha, interesting! I'm on macOS Catalina Version 10.15.7.

Removing the -u option from line 3 did the trick.

I'm not well versed in shell parameter expansions; could this be a solution?

nickjj commented 3 years ago

If you run bash --version what version do you get back?

In our case we're using ${@} which will let you pass in 0 or more arguments from our script to the command we're calling. This way you can naturally pass in multiple arguments to a command just as you would if you called it directly.

Setting a default is a reasonable idea with $1 because it's only dealing with 1 argument.

If we set "${@:-}" (an empty default) it's going to convert that to sending in "" which will make the underlying command fail. If we don't use quotes then it's not going to work as intended (it won't get parsed correctly).

chanana commented 3 years ago

running bash --version produces:

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

I understand what you mean with the ${@} vs the "${@}". This and this are great explanations for people looking at this in the future...

nickjj commented 3 years ago

I should probably bite the bullet and yank out the u flag. Having it not work for a decent chunk of Mac users isn't a good alternative to having unbound variable protection. I remember researching how to do this once while keeping it in but coming up empty handed. Going to give it a day or so to think about it and then either remove u or patch in a proper fix.

If anyone has any suggestions let me know.

chanana commented 3 years ago

If I understand the run file correctly, you have _dc which is combined with the various functions to run the commands. For example, if a user wanted to run lint their code with flake8, they would normally run

flake8 my_project

but since the website is inside the "web" docker container, you'd have to first specify docker-compose and -exec and -T which you've cleverly handled with _dc and cmd. The "{@}" in the flake8 function simply looks for additional arguments to flake8? But there is also an @ in the cmd command itself. As a naive question, what is the actual command being run when I type in ./run flake8 --version? Is it docker-compose exec web flake8 --version?

nickjj commented 3 years ago

What is the actual command being run when I type in ./run flake8 --version? Is it docker-compose exec web flake8 --version

Yep you got it. The only difference would be if you were running it in an environment that lacks a TTY then docker-compose exec -T web flake8 --version would be run instead.

nickjj commented 3 years ago

I decided to push the fix to the main branch. Here's the commit: https://github.com/nickjj/docker-django-example/commit/d6dc0b6d1f26add0437e4d1fa1d6f9c58abd9835

I'd rather have the examples work for everyone as soon as possible. Can always add u back at a later time if a proper workaround is discovered.

Thanks for the report. Going to close this.