Update setup script with newest template and merge with old script #10529

mkllnk · 2023-03-07T23:55:51Z

What? Why?

Follow up on Remove obsolete/redundant lines in setup/script #7624
Addresses comment Upgrade to Rails 7.0 #10440 (comment)

Rails has a default location for setup scripts and newer versions are idempotent. You can use them to update your app as well. It helps devs to run all needed commands after switching branches.

What should we test?

Dev Test

Run ./bin/setup and everything should work.
Run ./script/setup and it should work as well.

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

jibees

LGTM!

👌

Thanks! 😎

dacook

I like the idea to have a single, modern setup script. However I think we're losing some advantages of the old script.
Can we incorporate them into one?

Or what if we updated script/setup to first call bin/setup, then run the extra improvements on top of that. Then bin/setup can remain as the Rails template.

dacook · 2023-03-16T00:04:41Z

bin/setup

-  # Install JavaScript dependencies if using Yarn
-  # system('bin/yarn')
+  # Install JavaScript dependencies
+  system("yarn")


We do have a bin/yarn script, so why not use that as per the template? (although I'm not sure it really provides any benefit)

Ah, the next commit provides a clue. It's not quite as quick.

Oh, I didn't actually see that we had bin/yarn. I'll revise this commit.

script/setup

dacook · 2023-03-16T00:20:41Z

script/setup

-
-# Set up Ruby dependencies via Bundler
-if ! command -v bundle > /dev/null; then
-  ./script/install-bundler


This script actually checks the Bundler version, and installs new versions if needed, whereas bin/setup doesn't. I think it would be good to incorporate that to be more idempotent.

Somehow I thought that newer version of bundler automatically use the right version but that doesn't seem to be true. I misremembered the logic of https://bundler.io/guides/bundler_2_upgrade.html#version-autoswitch.

So yes, we should run this as well.

dacook · 2023-03-16T00:25:12Z

script/setup

-RUBY_VERSION=$(cat .ruby-version)
-
-# Check ruby version
-if ! ruby --version | grep $RUBY_VERSION > /dev/null; then


This check seems pretty useful. I would suggest we keep it.

I can also see possibility to improve and add a node version check also.
Maybe even conditionally run rbenv and nodenv if installed.

Good point. When I removed that I thought that we can just run script/rbenv-install but then I realised that this bin/setup is a Ruby script and won't run before installing ruby. And even when Ruby and rbenv are present then rbenv complaints that it can't find the right version.

So maybe that's another reason to keep script/setup to run the ruby installation.

mkllnk

Thank you for the thorough review. I will make some changes and ping you again for re-review before rewriting history.

mkllnk · 2023-03-17T03:06:47Z

bin/setup

-  # Install JavaScript dependencies if using Yarn
-  # system('bin/yarn')
+  # Install JavaScript dependencies
+  system("yarn")


Oh, I didn't actually see that we had bin/yarn. I'll revise this commit.

mkllnk · 2023-03-17T03:24:28Z

script/setup

-RUBY_VERSION=$(cat .ruby-version)
-
-# Check ruby version
-if ! ruby --version | grep $RUBY_VERSION > /dev/null; then


Good point. When I removed that I thought that we can just run script/rbenv-install but then I realised that this bin/setup is a Ruby script and won't run before installing ruby. And even when Ruby and rbenv are present then rbenv complaints that it can't find the right version.

So maybe that's another reason to keep script/setup to run the ruby installation.

mkllnk · 2023-03-17T03:29:47Z

script/setup

-
-# Set up Ruby dependencies via Bundler
-if ! command -v bundle > /dev/null; then
-  ./script/install-bundler


Somehow I thought that newer version of bundler automatically use the right version but that doesn't seem to be true. I misremembered the logic of https://bundler.io/guides/bundler_2_upgrade.html#version-autoswitch.

So yes, we should run this as well.

script/setup

dacook

Great, I think I found a bug though.

I note that bin/setup has some variations from the default now, which might not be obvious if updating for a new version in the future. But I doubt this matters.

script/setup

dacook

👍

I used the script from a new Rails 7 project.

The $ sign used to indicate shell commands. But with markdown it's obvious that we are entering commands. Github has a quick copy button for all code examples which used to copy the $ sign as well. Removing it allows to copy and paste easier with that button.

The command is switching to an unprivileged user which can't access /root and therefore there was a warning when executing. Adding `--login` to the sudo command switches to that user properly and avoids the warning.

People may use other ways to provide the right Ruby version but if they use rbenv then we can use it automatically.

Not just when it's missing.

And remove duplicate output. `bin/setup` is the Rails default for updating your environment after code updates and `script/setup` is our convenience script for the initial setup and starting with sample data.

dacook · 2023-04-03T05:00:19Z

I'm guessing just rebased?
I just came back to this and tested it, seems to work well.

Except that I think the sample_data task is not quite robust enough. My dev data seems to have broken it:

[ofn:sample_data] - cart order
rake aborted!
NoMethodError: undefined method `id' for nil:NilClass
/Users/dcook/projects/openfoodnetwork/lib/tasks/sample_data/order_factory.rb:66:in `create_order'
/Users/dcook/projects/openfoodnetwork/lib/tasks/sample_data/order_factory.rb:47:in `create_cart_order'

Looks like I have an outgoing exchange with no variants which has tripped it up. But I think that's a problem with that task, so all good! ✅

mkllnk · 2023-04-03T05:44:19Z

Thanks. Merging.

mkllnk added the dev-test A dev need to test this one label Mar 7, 2023

mkllnk self-assigned this Mar 7, 2023

mkllnk marked this pull request as ready for review March 7, 2023 23:56

jibees approved these changes Mar 9, 2023

View reviewed changes

dacook requested changes Mar 16, 2023

View reviewed changes

mkllnk commented Mar 17, 2023

View reviewed changes

mkllnk force-pushed the setup branch 2 times, most recently from e925887 to ce10adf Compare March 20, 2023 01:44

mkllnk requested a review from dacook March 20, 2023 01:55

dacook requested changes Mar 29, 2023

View reviewed changes

script/setup Show resolved Hide resolved

mkllnk requested a review from dacook April 3, 2023 01:32

dacook approved these changes Apr 3, 2023

View reviewed changes

mkllnk added 9 commits April 3, 2023 14:32

Update setup script with newest template

49dd1dd

I used the script from a new Rails 7 project.

Update JS dependencies with setup script

93ef95f

Speed up setup script when restarting rails

f12ee18

Avoid permission warning when running sudo as root

4999e23

The command is switching to an unprivileged user which can't access /root and therefore there was a warning when executing. Adding `--login` to the sudo command switches to that user properly and avoids the warning.

Install ruby automatically if rbenv is available

e5e5bf8

People may use other ways to provide the right Ruby version but if they use rbenv then we can use it automatically.

Update bundler version when necessary

4775240

Not just when it's missing.

Leverage bin/setup within script/setup

08f2254

And remove duplicate output. `bin/setup` is the Rails default for updating your environment after code updates and `script/setup` is our convenience script for the initial setup and starting with sample data.

Install our used bundler version

da0ef85

mkllnk force-pushed the setup branch from ed85c66 to da0ef85 Compare April 3, 2023 04:40

mkllnk merged commit de58aa1 into openfoodfoundation:master Apr 3, 2023

mkllnk deleted the setup branch April 3, 2023 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update setup script with newest template and merge with old script #10529

Update setup script with newest template and merge with old script #10529

mkllnk commented Mar 7, 2023 •

edited

Loading

jibees left a comment

dacook left a comment

dacook Mar 16, 2023

dacook Mar 16, 2023

mkllnk Mar 17, 2023

dacook Mar 16, 2023

mkllnk Mar 17, 2023

dacook Mar 16, 2023

mkllnk Mar 17, 2023

mkllnk left a comment

mkllnk Mar 17, 2023

mkllnk Mar 17, 2023

mkllnk Mar 17, 2023

dacook left a comment

dacook left a comment

dacook commented Apr 3, 2023 •

edited

Loading

mkllnk commented Apr 3, 2023

Update setup script with newest template and merge with old script #10529

Update setup script with newest template and merge with old script #10529

Conversation

mkllnk commented Mar 7, 2023 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

jibees left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook commented Apr 3, 2023 • edited Loading

mkllnk commented Apr 3, 2023

mkllnk commented Mar 7, 2023 •

edited

Loading

dacook commented Apr 3, 2023 •

edited

Loading