-
Notifications
You must be signed in to change notification settings - Fork 121
Change config file to optional file or environment variables #14
Change config file to optional file or environment variables #14
Conversation
README.md
Outdated
| `npm run setup` automates that for you but if you want to do it yourself rename `.contentful.json.sample` to `.contentful.json` and add your configuration in this file. | ||
| `npm run setup` automates that for you but if you want to do it yourself, create a file called `.env` in the project's root directory and add these three variables: | ||
|
|
||
| `CFSPACEID=`\<your space ID\> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we switch the names to match our example app pattern? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
gatsby-config.js
Outdated
|
|
||
| const contentfulDev = { | ||
| "host": "preview.contentful.com", | ||
| "spaceId": process.env.CFSPACEID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix indentation --> eslint --fix gatsby-config.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| } | ||
|
|
||
| const contentfulDev = { | ||
| "host": "preview.contentful.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should enforce preview for dev only. I'd go for non-prod environments, so you can easily deploy an staging server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on the NODE_ENV that is set by GatsbyJS. It is forced to "development" when "gatsby develop" is run and "production" when "gatsby build" is run, so on Netlify it will always be set to "production" and thus always use the delivery API.
Should we add an additional separate variable that allows the user to control whether they get preview or delivery? That would allow for a staging environment on Netlify to show unpublished content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note is that the Preview API does not have field validations enforced, unlike the Delivery API so it could be very easy to break a staging build simply because content that an editorial team is currently editing may have invalid fields: for instance, an entry may only have a title filled out, but other required fields may be absent.
We need to think about if we want to recommend using Preview API as a best practice for development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @axe312ger mentioned, now that Space Environments exist, a dev environment certainly makes sense for use while implementing new features in a Gatsby site. Previewing content is another story
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-source-contentful will need to be updated to support Space Environments before we could implement that here, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brampling it does support environment already in gatsbyjs/gatsby#5142
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other changes in this PR simply allow the use of environment variables with the existing workflow. Could we make the change from the preview API to Space Environments in a separate PR since it's a significant workflow change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree, keep the environments to a separate PR!
|
Great great, exactly what I wanted 🙈 😍 |
|
I'll make the requested changes. |
|
Let's wait for a review from @Khaledgarbaya and @Bloomca as this PR relates to work being done for a project atm |
|
Ok, I'll wait. 😁 |
|
thanks for the PR btw @brampling ! |
|
@Khaledgarbaya should I keep cleaning this up or leave it for now? |
|
Hey @brampling feel free to continue cleaning up |
axe312ger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but still waiting for @Khaledgarbaya or @Bloomca
|
I overlooked #18 which is already merged. @brampling did this PR everything you need? If no, you may reopen this one. |
I wasn't able to deploy this on Netlify without putting my API keys in a static file on Github which would be a Bad Thing.
I've changed setup.js to create a file called .env containing environment variables instead of creating the .contentful.json file. gatsby-config.js now looks for the .env file and if present, loads it with the dotenv module. It then uses the system environment variables CFSPACEID, CPATOKEN, and CDATOKEN for the config info. This means on a normal host I can keep the .env file in place and have the same workflow as the existing code. If I want though, I can delete the .env file and set the environment variables which means I can use it with Netlify CD without exposing API keys.