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

Misc: auth-devise, extracted frontend gem, sqlite #188

Merged
merged 14 commits into from Sep 7, 2022

Conversation

elia
Copy link
Member

@elia elia commented Aug 30, 2022

Summary

A few minor fixes and updates after the release of v3.2

Checklist

  • I have structured the commits for clarity and conciseness.
  • I have added relevant automated tests for this change.

@elia elia self-assigned this Aug 30, 2022
@mergify mergify bot added the needs changelog label Needs a label to determine the type of change. label Aug 30, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 30, 2022

It looks like this PR is missing a label to determine the type of change it introduces. The maintainer should add one of the following labels:

  • bug for bugfixes.
  • enhancement for new features and improvements.
  • documentation for documentation changes.
  • security for security patches.
  • removed for feature removals.
  • infrastructure for internal changes that should not go in the changelog.

Additionally, the maintainer may also want to add one of the following:

  • breaking for breaking changes.
  • deprecated for feature deprecations.

Once the correct labels have been set, simply remove the needs changelog label label from this PR so I can merge it.

@elia elia added the enhancement Improves an existing feature. label Aug 30, 2022
@@ -50,7 +50,7 @@ fi
cd ./sandbox
cat <<RUBY >> Gemfile
gem 'solidus', github: 'solidusio/solidus', branch: '$BRANCH'
gem 'solidus_auth_devise', '>= 2.1.0'
<%= "gem 'solidus_auth_devise', '>= 2.1.0'" unless file_name == 'solidus_auth_devise' %>
Copy link
Contributor

Choose a reason for hiding this comment

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

What about removing this line and switching --with-authentication to true in the solidus installer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I'll try that

Copy link
Member Author

Choose a reason for hiding this comment

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

@waiting-for-dev done, an explanation on why we didn't do it before is in the last commit, but basically it boils down to 3017ed6.

@elia elia force-pushed the elia/solidus-starter-frontend-support branch 4 times, most recently from 65942ce to 9dc5752 Compare September 1, 2022 15:12
@elia elia marked this pull request as draft September 2, 2022 10:16
@elia elia removed the needs changelog label Needs a label to determine the type of change. label Sep 2, 2022
@elia elia force-pushed the elia/solidus-starter-frontend-support branch from 988f708 to c9844fe Compare September 2, 2022 16:35
@elia elia marked this pull request as ready for review September 2, 2022 16:45
@@ -6,6 +6,10 @@ git_source(:github) { |repo| "https://github.com/#{repo}.git" }
branch = ENV.fetch('SOLIDUS_BRANCH', 'master')
gem 'solidus', github: 'solidusio/solidus', branch: branch

# The solidus_frontend gem has been pulled out since v3.2
gem 'solidus_frontend', github: 'solidusio/solidus_frontend' if branch == 'master'
Copy link
Contributor

Choose a reason for hiding this comment

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

As that's the template for extensions, I think we don't need to add solidus_frontend to the Gemfile at all. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intended as a live support gem for extension development, that means any extension is supposed to be regularly regenerating the support files (very similarly to what happens with rails app:update), so we should support the transition for existing gem that often rely on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I see it makes sense. Maybe we can detect if the file doesn't exist and then not add those lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you mean, is that ok if I reserve this change for a different PR?

Copy link
Member

Choose a reason for hiding this comment

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

With this change my extensions Gemfile suddenly has a dependency to solidus_frontend although it does not depend on it after I ran bundle exec rake. I would prefer that the extensions Gemfile would not get mutated on CIs like that. Is there a way to disable solidus_frontend completely? Ie. with SOLIDUS_FRONTEND=false

Copy link
Contributor

Choose a reason for hiding this comment

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

@tvdeyen, I'd also prefer not adding solidus_frontend for new extensions. However, to work around your issue, as that's generated code you can just remove the solidus_frontend Gemfile line, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

However, to work around your issue, as that's generated code you can just remove the solidus_frontend Gemfile line, right?

Exactly, that's the intent of having a generator instead of mandating a solution.

I would add that this whole setup is not covering the new frontend in any way and we'll probably need to come back to it once we start testing all extensions (that require it) with the new one.


echo
echo "🚀 Sandbox app successfully created for $extension_name!"
echo "🚀 Using $RAILSDB and Solidus $BRANCH"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to remove this line? It could be informative if you haven't specified the variables yourself.

Copy link
Member Author

Choose a reason for hiding this comment

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

No strong feelings, I just wanted to reduce the visual noise, but I'll re-add it with the additional info

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion about it, either, so do as you think it's better 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Ended up inlining usage and instruction messages near each other at the beginning of the script

echo "🚀 This app is intended for test purposes."
test -z "$DB" && echo "- Use 'export DB=[postgres|mysql|sqlite]' to control the DB adapter"
test -z "$SOLIDUS_BRANCH" && echo "- Use 'export SOLIDUS_BRANCH=[master|v3.2|...]' to control the Solidus version"
test -z "$SOLIDUS_FRONTEND" && echo "- Use 'export SOLIDUS_FRONTEND=[solidus_frontend|solidus_starter_frontend]' to control the Solidus frontend"
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, as it's for new extensions, maybe it's not worth it and we should just generate with solidus_starter_frontend. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's for supporting existing extensions rather than only creating new ones

@@ -183,7 +183,7 @@ def bundle_install
bundle_path = "#{gem_root}/vendor/bundle"

command = 'bundle install'
command += " --path=#{bundle_path.shellescape}" if File.exist?(bundle_path)
command = "env BUNDLE_PATH=#{bundle_path.shellescape} #{command}" if File.exist?(bundle_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just inline bundle install instead of overriding the command variable. I think it's less confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by a later commit bc2ecca5

@@ -50,7 +55,6 @@ fi
cd ./sandbox
cat <<RUBY >> Gemfile
gem 'solidus', github: 'solidusio/solidus', branch: '$BRANCH'
gem 'solidus_auth_devise', '>= 2.1.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the first commit from this PR to have a cleaner git history.

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Many thanks for all the improvements, @elia! I just left some comments.

Since Solidus v3.2 the frontend gem is in its own repo.
Default to the legacy frontend as it's the one used in most existing
extensions, and let the transition be opt-in.
Don't add solidus_auth_devise to the sandbox app if it's the current
extension.

In 3017ed6 solidus_auth_devise was added directly to the gemfile in
order to support solidus prior to v2.11. Since we don't need to
support it anymore we can safely use the official
`--with-authentication` option now.
Rails 7 doesn't work with an in-memory sqlite, and the extensions Orb
v0.4 supports a file-based executor which helps keeping the CI
footprint small.
The option is deprecated and now makes the command fail instead of
just printing a warning.
Rails started setting eager_load to true when the CI env var is set.

rails/rails@db0ee287eed
Using the --path CLI option was deprecated but now aborts the command,
used to just print a warning.
Also support the DEBUG env variable in addition to the `$DEBUG` global
ruby variable.
Be sure to include recent fixes.
@elia elia force-pushed the elia/solidus-starter-frontend-support branch from a47c8fd to b794ef0 Compare September 6, 2022 14:23
@mergify mergify bot merged commit de36a2d into master Sep 7, 2022
@mergify mergify bot deleted the elia/solidus-starter-frontend-support branch September 7, 2022 10:09
@tvdeyen
Copy link
Member

tvdeyen commented Nov 23, 2022 via email

@waiting-for-dev
Copy link
Contributor

Oh, I see what you mean. You're running it on an existing extension. I think you're right.

@tvdeyen
Copy link
Member

tvdeyen commented Nov 23, 2022

Oh, I see what you mean. You're running it on an existing extension. I think you're right.

@waiting-for-dev

Yes, it is an existing extension and I just updated it to support Solidus 3.2.

I have a default Rakefile in my extension:

require "bundler"
Bundler::GemHelper.install_tasks

require "solidus_dev_support/rake_tasks"
SolidusDevSupport::RakeTasks.install

task default: "extension:specs"

and the rake task suddenly started to add solidus_frontend to my local Gemfile

Exactly, that's the intent of having a generator instead of mandating a solution.

I would add that this whole setup is not covering the new frontend in any way and we'll probably need to come back to it once we start testing all extensions (that require it) with the new one.

@elia

Even for new extensions I do not think this is the right approach. Shouldn't we just point to the right gem in the gemspec of the extension instead, if it depends on frontend? Why mutate the Gemfile at all?

@elia
Copy link
Member Author

elia commented Nov 23, 2022

@tvdeyen no strings attached to that code, given the goal we can find alternative solutions to it:

The goal was to ensure we're grabbing the master version of solidus_frontend when fetching solidus itself from master. Also it was moving from the assumption that solidus will already depend on solidus_frontend, which AFAIK will be true until we reach v4.

Would adding require: false to that line solve the issue you bumped into?

@tvdeyen
Copy link
Member

tvdeyen commented Nov 23, 2022

@tvdeyen no strings attached to that code, given the goal we can find alternative solutions to it:

The goal was to ensure we're grabbing the master version of solidus_frontend when fetching solidus itself from master. Also it was moving from the assumption that solidus will already depend on solidus_frontend, which AFAIK will be true until we reach v4.

Would adding require: false to that line solve the issue you bumped into?

@elia

require: false on solidus_frontend?

I do not see why this should fix the issue with that the Rake task from solidus_dev_support adds the line to the Gemfile

What am I missing?

🤔

@tvdeyen
Copy link
Member

tvdeyen commented Nov 23, 2022

So, to recap what is happening (not just for me, but for all extensions that update their code to Solidus 3.2 and that uses solidus_dev_support)

bundle exec rake

is calling the default Rake task added by solidus_dev_support

# Rakefile
require "bundler"
Bundler::GemHelper.install_tasks

require "solidus_dev_support/rake_tasks"
SolidusDevSupport::RakeTasks.install

task default: "extension:specs"

That installs a new dummy app into the extensions and runs specs on it. So far, so good. With the change of this PR the rake task that install the dummy app suddenly adds

gem "solidus_frontend", "~> 3.2"

to the Gemfile of the extension.

It was not before this change.

Preferably it would not do that unless the extension is using solidus_frontend, but even then, the extensions author should be advised to change the gemspec instead. Because the extension needs to depend on the correct version of solidus_frontend if needed.

Am I missing something?

@elia
Copy link
Member Author

elia commented Nov 23, 2022

Oh I see, I'm not sure that this change is the culprit although it looks like it at first analysis. The change in this PR will only be applied to an existing extension if you run bundle exec solidus extension ., which will run the extension generator again and propose changes.

What you're seeing I think is most likely something related to the solidus installer.

Of course this is just an hypothesis, we need to verify 🕵️
@tvdeyen feel free to ping me on slack for a quick pairing session to sort this out 🙌

@tvdeyen
Copy link
Member

tvdeyen commented Nov 23, 2022

@elia my guess that this line is the culprit

https://github.com/solidusio/solidus_dev_support/pull/188/files#diff-f344e31271cf7c15d576fc481bc7a9ebee669b8e0093e6f799b986cb9fcbf4aeR94

I assume that the app generator is adding this line to the Gemfile. Should we remove that and advise extensions authors to depend on solidus_frontend in the gemspec instead?

@elia
Copy link
Member Author

elia commented Nov 24, 2022

@tvdeyen 🤔 I think I'm still failing to see how that is different from before… my assumption being that a solidus install has always depended on solidus_frontend, is there a way I can reproduce the error locally on some extension?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants