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

Don't run app config initializer for generator #1144

Merged
merged 7 commits into from
Jan 18, 2021

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jan 17, 2021

In #1097 a change was made to require that the Shopify environment variables were present. As this code is in an initializer, it also runs when the generators run, but at that point in time the environment variables may not yet be available (e.g. because dotenv-rails hasn't been added yet), causing an error.

Based on a suggestion in https://stackoverflow.com/a/32789186, we can check if the initializer is running from a generator or not.

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in docs/, if necessary
  • For security fixes, the Disclosure Policy must be followed.

"|| raise('Missing SHOPIFY_API_KEY')", shopify_app
assert_match "config.secret = ENV.fetch('SHOPIFY_API_SECRET', '').presence " \
"|| raise('Missing SHOPIFY_API_SECRET')", shopify_app
assert_match "config.api_key = ENV.fetch('SHOPIFY_API_KEY', '')", shopify_app
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than copy the exact updated template code into the test, I've simplified this to check for just the 'essential' part. Verifying the template code doesn't prove that the generator works – ideally we should generate an app as part of the test suite.

@andyw8 andyw8 force-pushed the andyw8/dont-run-app-config-for-generator branch from 5025cfe to bb2cb0d Compare January 17, 2021 16:31
@andyw8 andyw8 marked this pull request as ready for review January 17, 2021 16:38
@andyw8 andyw8 requested a review from a team as a code owner January 17, 2021 16:38
@andyw8 andyw8 force-pushed the andyw8/dont-run-app-config-for-generator branch from 1878a7a to b5fe5f7 Compare January 17, 2021 16:39
@andyw8 andyw8 merged commit b9d9dbc into master Jan 18, 2021
@andyw8 andyw8 deleted the andyw8/dont-run-app-config-for-generator branch January 18, 2021 14:20
@andyw8 andyw8 changed the title Don't run app config for generator Don't run app config initializer for generator Jan 18, 2021
config.shop_session_repository = 'Shop'
config.allow_jwt_authentication = <%= !with_cookie_authentication? %>
config.allow_cookie_authentication = <%= with_cookie_authentication? %>
unless defined? Rails::Generators
Copy link

@xiaoboa xiaoboa Jan 21, 2021

Choose a reason for hiding this comment

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

@andyw8 with this change, the reference of ShopifyApp.configuration in generator code(not template code) should also be removed, there is one at home_controller_generator.rb

      def embedded_app?
        ShopifyApp.configuration.embedded_app?
      end

the method embedded_app? is no longer needed any more.
Otherwise the generated code of HomeController by default will be an authenticated home_controller instead of the unauthenticated home_controller, which is conflict with the README.md

By default, this generator creates an unauthenticated home_controller and a sample protected products_controller.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point @xiaoboa, I'm actually investigating that myself. We still need need the method if we're creating non-embedded apps but I'll make sure that home_controller is not authenticated.

Thanks!

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.

3 participants