Related Linear issue here. This is part of a set of diffs that will allow ShellCheck to be added to the CI. See inline comments for specific details of the ShellCheck error/warning output.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Exclude hidden files and directories using -not -path find flag instead of piping to grep
Comment Actions
Additionally, this script is run during the Backup, Blob, and Tunnelbroker builds so if those workflows continue to pass CI we can reasonably assume that the script continues to work as expected.
I don't think that's true, we just build the image, but never run it.
'cd services && docker-compose build --no-cache blob-server'
This script is part of the CMD layer, so it's what is going to be ran if we do run it, but we don't
Taking a step back, this can eventually be replaced by just installing the service (e.g. make install), and running tunnelbroker or the related command without using this
| services/lib/docker/run_service.sh | ||
|---|---|---|
| 7 | This code can look intimidating at first glance, but find is much better than ls so here's my explanation at making it easier to parse! Overall, the code produces the same output as ls EXCEPT it preprends EXE_PATH to the front of each file/directory ls would output. Additionally, ls prints in human-readable format (so multiple files can be printed on the same line), but this find command will print each file/directory on a newline. This is OK, however, since the count of EXE (shown on line 8) is checked against 1 (from line 10):
And if it is not 1, the code will exit. Thus, the find and ls commands are equivalent in the case where there is only 1 non-hidden file/directory in EXE_PATH with regards to the count of the number of files. Note that like ls, this find command excludes hidden files and directories and is not recursive. Here's the technical details of this command:
This is included to exclude the . directory in the current directory. Read more: https://stackoverflow.com/a/13525005/13079675
This is included to make the find command not recursive, i.e., only search for files/directories in the current directory and no deeper.
This flag filters out hidden files and directories. Read more: https://askubuntu.com/a/318211 | |
| 15 | We no longer need the $EXE_PATH since find prepends the EXE_PATH to the front of each file it outputs. | |
| 7–8 ↗ | (On Diff #15237) | |
| 8 ↗ | (On Diff #15237) | |
| 15 ↗ | (On Diff #15237) | |
Comment Actions
This script is part of the CMD layer, so it's what is going to be ran if we do run it, but we don't
I see! That makes sense, I saw it in the Dockerfiles and assumed it was run.
Taking a step back, this can eventually be replaced by just installing the service (e.g. make install), and running tunnelbroker or the related command without using this
Is that something we want to do? I don't have much context on this script's usage.
| services/lib/docker/run_service.sh | ||
|---|---|---|
| 7 | Appreciate the super thorough explanation here! | |
Comment Actions
Taking a step back, this can eventually be replaced by just installing the service (e.g. make install), and running tunnelbroker or the related command without using this
@jon, is this something you'd like to do? I've made a Linear issue here. Feel free to cancel/backlog/whatever if it's not something we need to do.
So should the test plan be updated?
Updated it, but just by removing the current one. I still think we should add a more thorough test plan, so I'm adding @jon as blocking for his feedback, since this was confusing to me:
This script is part of the CMD layer, so it's what is going to be ran if we do run it, but we don't
How do we get it to run so I can test it? Sorry, not very experienced with Docker.
Comment Actions
@jon, is this something you'd like to do? I've made a Linear issue here. Feel free to cancel/backlog/whatever if it's not something we need to do.
I think we can do that conversation on linear. But the diff here should still go in