Refactor bash scripts with some new updates #482

jwoertink · 2020-03-21T20:44:56Z

This has evolved a bit from it's first iteration. This PR refactors the setup script a bit and adds in some new stuff.

Adds new system_check script which is added to your Procfile.dev. This can check for things that need to run before your app is booted each time.
Moves the setup checks from setup to system_check. Now setup just installs what it needs.
Added some abstractions to clean up the setup script which moves those abstractions to a new helper directory
Added 2 new text_helpers and function_helpers to make writing bash easier because let's face it.. it's hard

…tart writing your checks.

src/web_app_skeleton/Procfile.dev.ecr

src/web_app_skeleton/script/system_check

paulcsmith

I like this approach and the docs are awesome. I have some thoughts on how we might use this to share some common checks in both script/setup and script/system_check!

For example, right now we check a bunch of stuff in script/setup, but I think those checks could/should go in system_check and script/setup calls the system_check script. This way when you run Lucky Dev and you forget script/setup you still get a lot of nice errors if it can't find psql, crystal, yarn etc.

The only problem is in the script/setup you may not have the redis/elastic/whatever running and it may not make sense to require that at the beginning of the setup script...but maybe it does since if you need those for the app to work then it might make sense to check they are running in the setup script too...I think it'd work and be nice. Thoughts?

src/web_app_skeleton/script/system_check

src/web_app_skeleton/Procfile.dev.ecr

paulcsmith · 2020-03-21T21:30:12Z

I'm very excited about having a convention for where to put these checks. In almost every codebase I've been on there are notes in the README (if I'm lucky) and checks in setup (again, if I'm lucky haha).

Having a clear place will really help with this, especially if we have some nice default ones like crystal, yarn, postgres, etc. checks

…er to understand.

jwoertink · 2020-03-27T15:52:53Z

Just put this in to my project, and it's working out great

[08:50AM] app (develop)$ lucky dev
system       | Tmux socket name: overmind-vip-YKZUn_NBtrxnD_xJW4ZavL
system       | Tmux session ID: vip
system       | Listening at /Users/jeremywoertink/Sites/app/.overmind.sock
web          | Started with pid 3093...
check_system | Started with pid 3092...
check_system | Could not connect to Redis at 127.0.0.1:6379: Connection refused
check_system | There is a problem with your system setup:
check_system |
check_system |    Redis is not currently running. Be sure to boot it first.
check_system |
web          | Beginning to watch your project
web          | Compiling...
check_system | Exited
web          | Interrupting...
web          | Exited
[08:50AM] app (develop)$
[08:50AM] app (develop)$
[08:50AM] app (develop)$ brew services start redis
==> Successfully started `redis` (label: homebrew.mxcl.redis)
[08:50AM] app (develop)$
[08:50AM] app (develop)$
[08:50AM] app (develop)$ lucky dev
system       | Tmux socket name: overmind-vip-QcVN00O4DPDY2h38uSsunr
system       | Tmux session ID: app
system       | Listening at /Users/jeremywoertink/Sites/app/.overmind.sock
check_system | Started with pid 3397...
web          | Started with pid 3398...
check_system | PONG
web          | Beginning to watch your project
web          | Compiling...
web          | Done compiling
web          | Listening on http://0.0.0.0:5100
web          |
web          |
web          |    🎉 App running at http://0.0.0.0:5100
web          |
web          |

The one I'm using is:

redis-cli ping
if [ $? -ne 0 ]  # Check the exit_code of the previous task. If not equal to 0, then print_error
then
  print_error "Redis is not currently running. Be sure to boot it first."
fi

jkthorne · 2020-03-27T18:18:53Z

I really like this idea

jwoertink · 2020-03-27T18:19:14Z

Ok, after bashing my head against this code, I was able to get some other stuff working as well specific for my apps. I wonder if this should include some bash guides, or helpful tips? Maybe even a few helper methods to check some things?

paulcsmith

Yes totally. A short guide with examples would be amazing. I think that could probably go on the website with links to it. Otherwise this file will be YUGE

Also did you get a chance to look at the comment about putting the checks we have in script/setup in script/system_check instead? Stuff like checking for process runner, yarn, pg can go here. Then in script/setup we call this script.

Would be super cool so you could have those checks in both the setup and when starting the app! Thoughts?

src/web_app_skeleton/script/check_system

jwoertink · 2020-03-27T20:41:25Z

Ok, after this is merged in, I'll open up an issue about writing some docs on it.

I was thinking about the setup stuff coming over to this, but had a concern. I originally saw this as purely a way to check and make sure things that need to be running for you to work on the app are actually running. I'm wondering if it would slow down the boot time if it has to check these on every boot:

is process runner installed? if this is called from lucky dev, then obviously it is.
is yarn installed?
is postgres installed?
is postgres running?
is crystal installed?
maybe is crystal the right version?
whatever else....

To me, these make sense to check when you setup your app, but once you've done that, you don't need to check them again. Unless you randomly uninstalled yarn or pg? but that feels edge casey...

Let me know if I'm just over thinking this 😅 I guess we could throw them in and try it out, if it gets clunky, we just rip it out. Pre 1.0 is the time to experiment 😆

paulcsmith · 2020-03-27T20:46:24Z

@jwoertink I think that those checks are super fast typically, also system_check will run in a separate process so will not block yarn or crystal compilation. The other part of this is what if you have a new check in setup because someone added elasticsearch to the setup checks. If you already had the app setup you'd start with lucky dev and unless they also checked in check_system they would not know about it.

The other part is that sometimes people just don't run the setup script 🤣 so having this would be an extra layer of helpful checks.

Does that make sense?

EDIT: Actually one downside here is the extra output. I'd say the checks for yarn/crystal/pg should go in system_check, but process runner probably doesn't need to. So I think anything at the top of script/setup that doesn't output anything on success would be a good candidate to put in check_system

jwoertink · 2020-03-27T20:50:08Z

ah! Ok, if you have a new check in setup because someone added elasticsearch is what sold me. I've had that happen a few times. Gone to other devs like "this won't boot", then get "oh yeah, you have to brew install whatever first...". 😐

Ok, I'll add that in... Also, side note, you've called it system_check twice now which makes me wonder if my original naming was better 😛

Co-Authored-By: Paul Smith <paulcsmith@users.noreply.github.com>

paulcsmith · 2020-03-27T21:05:25Z

Yeah I noticed that. I think you were right. I intuitively call it system_check and seems you did too so we can switch it back. My bad! 🤣

jwoertink · 2020-03-27T21:17:16Z

Oh, here's a question. In the setup script, we create the DB, then check to see we can connect before migrating. We are also saying postgres has to be connected before booting. It's a whole chicken an egg deal now when running a setup because it'll be like:

When running setup

check connection.. fail
create db
check connection... ok
migrate

When running lucky dev, it won't matter. Should the setup script just catch that first error and skip past it?

jwoertink · 2020-03-27T21:33:20Z

maybe I can just pass in a flag on whether or not it should be checked. Add it in the Procfile, but tell it no for setup

paulcsmith · 2020-03-27T21:41:10Z

I’m thinking we don’t do the verify Db in the system check and only do it in setup so the script doesn’t get too complex. Kind of a bummer to not have that but I think it’ll be ok since we have good errors when it can’t connect

…anyway. system_check is now an ECR template to add additional scripts if you generate a browser app. Moving all of the system_check stuff from the setup script and cleaning up the setup script

paulcsmith

Looks awesome. Very stoked about this. A few suggestions, but looks good to merge when you're ready

src/web_app_skeleton/script/setup.ecr

src/web_app_skeleton/script/system_check.ecr

jwoertink · 2020-03-28T20:30:58Z

src/web_app_skeleton/script/helpers/function_helpers

+#     echo "do linux stuff"
+#   fi
+is_linux() {
+  if [[ "$OSTYPE" == "linux"* ]]; then


It seems most people I asked get back linux-gnu on arch, debian, or other flavors. I didn't actually test this though, but I assume it works.

src/web_app_skeleton/script/setup.ecr

jwoertink · 2020-03-28T20:41:35Z

src/web_app_skeleton/script/system_check.ecr

+#   print_error "Redis is not running."
+# fi
+
+


My thought was having a very clear place to put your own custom stuff. This file is already really busy, so if you're new to bash, then it might not be apparent where you'd do stuff, or what you'd do.

src/web_app_skeleton/script/system_check.ecr

jwoertink · 2020-03-28T20:43:19Z

Ok, I added a bunch of goodies here. I threw this in to a fresh 0.19 app and it worked great. If y'all wanna give it a once over that'd be cool, but I think it's ready to go now.

paulcsmith

I love the new function helpers. So nice to have those abstracted away. I always have to look up this stuff so it’s nice to have it right there.

A few small suggestions. The most important is moving shell to the Procfile since it is the only thing that needs it. I think that should work 🤞

src/web_app_skeleton/Procfile.dev.ecr

src/web_app_skeleton/script/helpers/function_helpers

paulcsmith · 2020-03-28T21:04:15Z

src/web_app_skeleton/script/helpers/function_helpers

+# Prints error and exit.
+# example:
+#   print_error "Redis is not running. Run it with some_command"
+print_error() {


I think this makes sense in the text helpers script but up to you. I don’t feel strongly about it

I thought that at first, but because it does an exit, and has specific text in it, it felt more like a function to me

…ove it there. This way if you want to call the system_check from some other script, you can.

* adding a system_check script for pre boot tasks in development. * Updating the system_check with some better examples, and a place to start writing your checks. * I think you can do this... * actually use the system_check to make the examples of what to do easier to understand. * renamed system_check to check_system * Update src/web_app_skeleton/script/check_system Co-Authored-By: Paul Smith <paulcsmith@users.noreply.github.com> * Update src/web_app_skeleton/script/check_system Co-Authored-By: Paul Smith <paulcsmith@users.noreply.github.com> * Update src/web_app_skeleton/script/check_system Co-Authored-By: Paul Smith <paulcsmith@users.noreply.github.com> * calling check_system system_check again because we kept call it that anyway. system_check is now an ECR template to add additional scripts if you generate a browser app. Moving all of the system_check stuff from the setup script and cleaning up the setup script * huge refactor of script setup system. Added some new bash helpers * Since the Procfile is the only thing that needs that SHELL, we just move it there. This way if you want to call the system_check from some other script, you can. Co-authored-by: Paul Smith <paulcsmith@users.noreply.github.com>

jwoertink added 3 commits October 25, 2019 12:57

adding a system_check script for pre boot tasks in development.

5d2636a

Merge branch 'master' into features/pre_boot_script

3230aff

Updating the system_check with some better examples, and a place to s…

084ef43

…tart writing your checks.

jwoertink commented Mar 21, 2020

View reviewed changes

src/web_app_skeleton/Procfile.dev.ecr Outdated Show resolved Hide resolved

jwoertink commented Mar 21, 2020

View reviewed changes

src/web_app_skeleton/script/system_check Outdated Show resolved Hide resolved

paulcsmith reviewed Mar 21, 2020

View reviewed changes

jwoertink added 3 commits March 26, 2020 14:13

I think you can do this...

c0baab2

actually use the system_check to make the examples of what to do easi…

5aa7cbb

…er to understand.

renamed system_check to check_system

4a08294

paulcsmith reviewed Mar 27, 2020

View reviewed changes

jwoertink and others added 3 commits March 27, 2020 13:53

Update src/web_app_skeleton/script/check_system

d226ed3

Co-Authored-By: Paul Smith <paulcsmith@users.noreply.github.com>

Update src/web_app_skeleton/script/check_system

2616c17

Co-Authored-By: Paul Smith <paulcsmith@users.noreply.github.com>

Update src/web_app_skeleton/script/check_system

00a9e96

Co-Authored-By: Paul Smith <paulcsmith@users.noreply.github.com>

calling check_system system_check again because we kept call it that …

a12276b

…anyway. system_check is now an ECR template to add additional scripts if you generate a browser app. Moving all of the system_check stuff from the setup script and cleaning up the setup script

paulcsmith reviewed Mar 28, 2020

View reviewed changes

jwoertink changed the title ~~Features/pre boot script~~ Refactor bash scripts with some new updates Mar 28, 2020

huge refactor of script setup system. Added some new bash helpers

17d65c0

jwoertink commented Mar 28, 2020

View reviewed changes

src/web_app_skeleton/script/setup.ecr Outdated Show resolved Hide resolved

jwoertink commented Mar 28, 2020

View reviewed changes

src/web_app_skeleton/script/system_check.ecr Outdated Show resolved Hide resolved

paulcsmith approved these changes Mar 28, 2020

View reviewed changes

Since the Procfile is the only thing that needs that SHELL, we just m…

06a89a5

…ove it there. This way if you want to call the system_check from some other script, you can.

jwoertink merged commit ee7699b into master Mar 28, 2020

jwoertink deleted the features/pre_boot_script branch March 28, 2020 22:48

jwoertink mentioned this pull request Mar 28, 2020

Clean up new bash scripts #495

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor bash scripts with some new updates #482

Refactor bash scripts with some new updates #482

jwoertink commented Mar 21, 2020 •

edited

Loading

paulcsmith left a comment

paulcsmith commented Mar 21, 2020

jwoertink commented Mar 27, 2020

jkthorne commented Mar 27, 2020

jwoertink commented Mar 27, 2020

paulcsmith left a comment

jwoertink commented Mar 27, 2020

paulcsmith commented Mar 27, 2020 •

edited

Loading

jwoertink commented Mar 27, 2020

paulcsmith commented Mar 27, 2020

jwoertink commented Mar 27, 2020

jwoertink commented Mar 27, 2020

paulcsmith commented Mar 27, 2020 •

edited

Loading

paulcsmith left a comment

jwoertink Mar 28, 2020

jwoertink Mar 28, 2020

jwoertink commented Mar 28, 2020

paulcsmith left a comment

paulcsmith Mar 28, 2020

jwoertink Mar 28, 2020

Refactor bash scripts with some new updates #482

Refactor bash scripts with some new updates #482

Conversation

jwoertink commented Mar 21, 2020 • edited Loading

paulcsmith left a comment

Choose a reason for hiding this comment

paulcsmith commented Mar 21, 2020

jwoertink commented Mar 27, 2020

jkthorne commented Mar 27, 2020

jwoertink commented Mar 27, 2020

paulcsmith left a comment

Choose a reason for hiding this comment

jwoertink commented Mar 27, 2020

paulcsmith commented Mar 27, 2020 • edited Loading

jwoertink commented Mar 27, 2020

paulcsmith commented Mar 27, 2020

jwoertink commented Mar 27, 2020

jwoertink commented Mar 27, 2020

paulcsmith commented Mar 27, 2020 • edited Loading

paulcsmith left a comment

Choose a reason for hiding this comment

jwoertink Mar 28, 2020

Choose a reason for hiding this comment

jwoertink Mar 28, 2020

Choose a reason for hiding this comment

jwoertink commented Mar 28, 2020

paulcsmith left a comment

Choose a reason for hiding this comment

paulcsmith Mar 28, 2020

Choose a reason for hiding this comment

jwoertink Mar 28, 2020

Choose a reason for hiding this comment

jwoertink commented Mar 21, 2020 •

edited

Loading

paulcsmith commented Mar 27, 2020 •

edited

Loading

paulcsmith commented Mar 27, 2020 •

edited

Loading