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

PR to hang a review on. #1

Open
wants to merge 8 commits into
base: review
Choose a base branch
from
Open

PR to hang a review on. #1

wants to merge 8 commits into from

Conversation

firebus
Copy link
Owner

@firebus firebus commented Sep 3, 2021

Ditto.

if [ -z $1 ]; then
containersArray=$(docker ps | awk '{if(NR>1) print $NF}')
else
containers="$1"
Copy link
Owner Author

Choose a reason for hiding this comment

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

looks like this is maybe a debug feature? but now $containers doesn't seem to be used anywhere, so it's kind a dead end. maybe remove?

Copy link

@mrjones-plip mrjones-plip Sep 28, 2021

Choose a reason for hiding this comment

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

Nice catch! fixed in f4e4f92 & 7e19d54


}

main_loop -t 1.2 $@
Copy link
Owner Author

Choose a reason for hiding this comment

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

what does the -t 1.2 and $@ do here?

Choose a reason for hiding this comment

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

these are all the default arguments to bashsimplecurses

@@ -0,0 +1,98 @@
#!/usr/bin/env bash
Copy link
Owner Author

Choose a reason for hiding this comment

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

if you're looking to gold-plate this: https://kvz.io/bash-best-practices.html

Choose a reason for hiding this comment

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

this is a great resource - thank you! I suspect this will be a "one and done" kinda project, but if I go back an do a refactor or for my next project, I'll keep https://bash3boilerplate.sh/ in mind!

# "system_profiler" exists only on MacOS, if it's not here, then run linux style command for
# load avg. Otherwise use MacOS style command
if ! command -v "system_profiler" &>/dev/null; then
awk '{ print $1 " " $2 " " $3 }' </proc/loadavg
Copy link
Owner Author

Choose a reason for hiding this comment

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

how does this script behave if awk is not available?

Choose a reason for hiding this comment

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

poorly! we can be defensive here. see 281286a for such a defense!

Choose a reason for hiding this comment

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

@ch4lox - you'd suggested using docker ps --format '{{json .}}' along with jq. I love this! However, it was gonna be a lot more work and add another item to the fix in 281286a. Given this works "out of the box" with out any extras, Imma pass on it now.

Thanks @firebus for letting me use your PR as code review so I can give @ch4lox feedback on his Signal as feedback from the other month 🙃

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