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

Only skip environment variables from initializer for generators #1155

Merged
merged 1 commit into from
Jan 22, 2021

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Jan 21, 2021

Once we started skipping the initializer for generators (#1144), we ended up in a situation where the default config generated by the template and the default values in ShopifyApp configuration had opposing values, therefore causing new apps to generate an authenticated home_controller.rb file rather than its unauthenticated counterpart because cookie auth is still the default.

This PR addresses that by only skipping the values that depend on environment variables when running generators so that the actual config of the app still applies on generator runs and creates consistent apps.

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.

@paulomarg paulomarg force-pushed the embedded_by_default_config branch 2 times, most recently from 7730a9a to 96df0c3 Compare January 21, 2021 19:27
@paulomarg paulomarg marked this pull request as ready for review January 21, 2021 19:31
@paulomarg paulomarg requested a review from a team as a code owner January 21, 2021 19:31
@paulomarg paulomarg changed the title Change configuration to make apps embedded by default Only skip environment variables from initializer for generators Jan 21, 2021
@paulomarg paulomarg requested a review from andyw8 January 21, 2021 20:55
config.allow_jwt_authentication = <%= !with_cookie_authentication? %>
config.allow_cookie_authentication = <%= with_cookie_authentication? %>

if defined?(Rails::Server) || defined?(Rails::Console) || (File.basename($PROGRAM_NAME) === 'rake')
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Rails 5, some rake commands are also callable with rails, e.g. rails routes instead of rake routes – can we check this is handled correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the comparison can be ==, the === is very rarely needed in Ruby.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm starting to wonder if the raise call is worth going through all of this trouble. We could simply let the keys be empty if the value isn't set rather than checking for all possible scenarios.

What if we just log something if Server is defined and let things slide with an empty key for other cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

Starting the server with missing env vars would be the most common mistake. so how about we raise on that, but ignore the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent point, I agree. Making it happen 🙂

@paulomarg paulomarg merged commit 128ff17 into master Jan 22, 2021
@paulomarg paulomarg deleted the embedded_by_default_config branch January 22, 2021 19:50
@paulomarg paulomarg temporarily deployed to rubygems January 22, 2021 21:53 Inactive
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.

None yet

3 participants