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

Fix the install generator #3777

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

seand7565
Copy link
Contributor

@seand7565 seand7565 commented Sep 25, 2020

ref #3775

Fixes the issues with the install generator by bundling after adding the
extensions to the gemfile, stopping spring, and then running the install
generators for each of the installed extensions.

Currently this works a bit wonky as the extensions will ask if you want
to run migrations, and that's not desirable as migrations are already run
after these extensions are installed. Will fix and update shortly, just
wanted to get this into a PR!

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@seand7565
Copy link
Contributor Author

Currently, it's impossible to run extension installers
A. without running migrations and
B. without being asked whether or not you want to run migrations

Ideally, with the overall Solidus installer, the user would be able to just run it and have a working solidus store, while installing the default extensions. Currently that's not possible.

The two solutions I can think of are:

  1. Running the install generators separately. In previous versions of Solidus, we had to run the install generator for solidus_auth_devise separately. We could go back to that functionality - although it gets weird when we have multiple payment methods able to be installed.

  2. Change the way we're migrating in extension installers.

Here's an example of an extension installer - https://github.com/solidusio/solidus_auth_devise/blob/8b65779f02a4fe46d81bbf0c78347b39f8f93205/lib/generators/solidus/auth/install/install_generator.rb#L23

If you pass in --auto-run-migrations=false it will still ask you if you want to run migrations. We could update that to a run-migrations flag and if it's not explicitly set, then ask if they want to run migrations. We could also just default to automatically running migrations and have a flag that turns that functionality off. I think the use case of not running migrations after installation is pretty small (pretty much just useful in the context of this PR AFAIK).

However that requires making changes to a lot of extensions, and to solidus_dev_support, which is a lot of work. We'd also have to create new releases for some of these extensions after doing that. (though solidus_auth_devise needs a new release anyways).

What do you think? @kennyadsl @elia

@kennyadsl
Copy link
Member

What if you run:

run "rails generate #{plugin_generator_name} --auto-run-migrations=false"

instead of:

generate "#{plugin_generator_name} --auto-run-migrations=false"

Will the message be prompted anyway?

@kennyadsl
Copy link
Member

kennyadsl commented Sep 28, 2020

Also, I think the generate syntax is wrong. Should be something like:

generate "#{plugin_generator_name}", "--auto-run-migrations=false"

@seand7565
Copy link
Contributor Author

Unfortunately the migration question gets asked regardless of syntax - I tried both ways. Thanks for pointing out the generate syntax!

The problem is this line:

run_migrations = options[:auto_run_migrations] || ['', 'y', 'Y'].include?(ask('Would you like to run the migrations now? [Y/n]'))

if you set auto_run_migrations to false, then the question will always be asked.

@seand7565
Copy link
Contributor Author

Maybe it's not that big of a deal though, since there are several other prompts during installation. It is a bit weird to ask to migrate 3 times during the initial installation, but probably easier to keep it that way than change every extension that uses that line. :P

@elia
Copy link
Member

elia commented Sep 30, 2020

@seand7565 I think it's worth fixing it, not for the seasoned Solidus dev, but for everyone trying to install it for the first time, who I suspect would be rather confused by that.

I also hope to come back to #3743 and finish it to further improve the usability of the installer.

@kennyadsl
Copy link
Member

@seand7565 got it. If we don't want to auto-run migrations the message will be prompted.

I'd start modifying these two extensions for now, adding another parameter that force not asking and not migrating. Maybe we can move this shared logic into a solidus_dev_support module that we can include in both and gradually adopt in other extensions when needed?

@seand7565
Copy link
Contributor Author

@seand7565 got it. If we don't want to auto-run migrations the message will be prompted.

I'd start modifying these two extensions for now, adding another parameter that force not asking and not migrating. Maybe we can move this shared logic into a solidus_dev_support module that we can include in both and gradually adopt in other extensions when needed?

Yeah, let's do that! I have a PR on SPCP that handles this change, take a look if you get a chance. If it looks good, I'll also port it into solidus_devise - at which point this PR should be mergeable (though we'll need a re-release of both SPCP and solidus_devise) solidusio/solidus_paypal_commerce_platform#94

Then we can add it into solidus_dev_support so any new extensions are built with this change.

I opted for a skip-migrations option because I think it makes the most sense here - I thought copying the new syntax from #3743 might be acceptable too, but it would still run the migrations 3 times, and I'd prefer to skip the migrations entirely until the final migration happens as part of the apps setup.

@seand7565
Copy link
Contributor Author

Here's the change in solidus_auth_devise: solidusio/solidus_auth_devise#194

@seand7565 seand7565 force-pushed the fix_install_generator branch from 2952f54 to 6f6bd2e Compare October 5, 2020 18:23
@seand7565
Copy link
Contributor Author

The devise PR needs to be merged, then a new release cut.
The SPCP PR just needs a new release.

But other than that, this PR is good to go, so I'll un-draft it. Once a new release is cut for both extensions, this should fix the installers issues.

@seand7565 seand7565 marked this pull request as ready for review October 5, 2020 18:25
@kennyadsl
Copy link
Member

@seand7565 I just released solidus_auth_devise with the installer enhancement. Can you take care of the SPCP one or do you need my assistance there?

@seand7565
Copy link
Contributor Author

@kennyadsl It's been released! 🎉 This PR should be good to go. I'll give it a quick rebase first though.

ref solidusio#3775

Fixes the issues with the install generator by bundling after adding the
extensions to the gemfile, stopping spring, and then running the install
generators for each of the installed extensions.

Currently this works a bit wonky as the extensions will ask if you want
to run migrations, and that's not desirable as migrations are already run
after these extensions are installed. Will fix and update shortly, just
wanted to get this into a PR!
@seand7565 seand7565 force-pushed the fix_install_generator branch from 6f6bd2e to 165321d Compare October 14, 2020 13:39
@kennyadsl kennyadsl merged commit ee05883 into solidusio:master Oct 15, 2020
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.

6 participants