Skip to content

Pre-run setup. #9

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Pre-run setup. #9

wants to merge 11 commits into from

Conversation

tkopczuk
Copy link
Contributor

@tkopczuk tkopczuk commented Oct 2, 2019

No description provided.

@tkopczuk tkopczuk requested a review from Stosiu October 2, 2019 10:24
Copy link

@Stosiu Stosiu left a comment

Choose a reason for hiding this comment

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

I like this feature a lot! 🎉 I left some of my thoughts about how we can improve the user experience.

README.md Outdated
Comment on lines 56 to 59
*NOTE*: setup scripts *must* be idempotent (able to run multiple times without adverse effects), as they will run multiple times - on each change to the setup scripts.

*NOTE 2*: each setup script will be run up to 10 times, with a delay of 3 seconds (determined by it's exit code).

Copy link

Choose a reason for hiding this comment

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

Note #1 It's quite an important, IMO. Maybe instead of saying it in a note, we can add it to the description somewhere at the beginning?

@tkopczuk
Copy link
Contributor Author

tkopczuk commented Oct 2, 2019

@Stosiu Please do dbl. check the rephrased version. Loved your comments.

Copy link

@Stosiu Stosiu left a comment

Choose a reason for hiding this comment

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

The code looks good and I like the rephrased text!

I left some more thoughts about the new texts but don't be bothered about them that much if you think that they don't make much sense (they are my personal preferences rather than an objective point of view arguments).

The only thing to fix is a typo in line 46 in README.md: accomodate -> accommodate.

README.md Outdated
## Services pre-run setup
Some services will need to perform a certain set of actions before they are able to start successfully. This might include eg. running migrations, setting up fixtures, etc.

To accomodate this, this script will run all the setup scripts, every time they change since the last setup run (as per last git modification timestamp).
Copy link

@Stosiu Stosiu Oct 3, 2019

Choose a reason for hiding this comment

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

I always feel weird reading docs written this way 😅 this script will run all the setup scripts well okay, but at this point in the text I have no clue how to set them up, how they should look like or even if they already exist or not.

Copy link

Choose a reason for hiding this comment

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

I prefere aproches like:

Some services will need to perform a certain set of actions before they are able to start successfully. This might include eg. running migrations, setting up fixtures, etc.

To accommodate this, create a ./setup directory with .sh files you would like to run. Scripts are executed in alphabetical order as per bash. Scripts will run every time they change since the last setup run (as per last git modification timestamp).

Example setup scripts initialization

mkdir -p ./setup
touch ./setup/initialize-database.sh

(...)


Setup step will run right after `up` command.

Setup can be forced (regardless of the timestamps) by running:
Copy link

Choose a reason for hiding this comment

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

Side note: While reading this I asked myself if this updates timestamp and it does.

Co-Authored-By: Aleksander Stós <aleksander.stos@gmail.com>
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