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

Enforce presence of application key and secret during initialization #1097

Merged
merged 3 commits into from
Oct 22, 2020

Conversation

tjoyal
Copy link
Member

@tjoyal tjoyal commented Oct 19, 2020

Thoughts on making it explicit when the key is not set when it is expected for it to be present so it fail fast?

Thoughts on making it explicit when the key is not set when it is expected for it to be present so it fail fast?
@andyw8
Copy link
Contributor

andyw8 commented Oct 19, 2020

I support this.

@tjoyal tjoyal marked this pull request as ready for review October 19, 2020 23:15
@tjoyal tjoyal requested a review from a team as a code owner October 19, 2020 23:15
@tjoyal
Copy link
Member Author

tjoyal commented Oct 19, 2020

I ended up with this in my app, not sure if we prefer that:

  config.api_key = ENV.fetch('SHOPIFY_API_KEY', '').presence || raise("Missing SHOPIFY_API_KEY")
  config.secret = ENV.fetch('SHOPIFY_API_SECRET', '').presence || raise("Missing SHOPIFY_API_SECRET")

I use presence as I use dotenv and have this so the keys are git versioned but not the secrets:

# .env
SHOPIFY_API_KEY=
SHOPIFY_API_SECRET=

# .env.local
SHOPIFY_API_KEY=the-actual-key
SHOPIFY_API_SECRET=the-actual-key

# .gitignore
.env.local

@paulomarg
Copy link
Contributor

I actually like throwing a more specific error if one of the keys isn't present, it can be a bit of a pain point when apps start up without the necessary envs and fail later on for seemingly random reasons.

@tjoyal tjoyal merged commit 8dc099e into master Oct 22, 2020
@tjoyal tjoyal deleted the tjoyal-patch-1 branch October 22, 2020 16:24
@jgray7019 jgray7019 temporarily deployed to rubygems December 10, 2020 20:54 Inactive
@andyw8
Copy link
Contributor

andyw8 commented Jan 6, 2021

@tjoyal I think this may be causing some problems: Shopify/shopify-cli#1006

@tjoyal
Copy link
Member Author

tjoyal commented Jan 7, 2021

@andyw8 To make sure we are on the same page, I do not think we are challenging here the fact that is raises but rather the low quality error message? Seems like a fair ask.

@andyw8
Copy link
Contributor

andyw8 commented Jan 7, 2021

@tjoyal The error message is a secondary concern. But I think enforcing that the env vars are always present might have impacted how the generator runs. We're still investigating, no need to take action yet.

@andyw8
Copy link
Contributor

andyw8 commented Jan 7, 2021

Shopify/shopify-cli#1026

@andyw8
Copy link
Contributor

andyw8 commented Jan 17, 2021

#1144

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.

5 participants