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

Self Documenting Braintree Environment Variable #2008

Merged
merged 17 commits into from
Dec 6, 2019

Conversation

supernova-at
Copy link
Contributor

@supernova-at supernova-at commented Nov 27, 2019

Description

The CHECKOUT_BRAINTREE_TOKEN environment variable defined in envVarDefinitions.json provided a default value which hid the fact that developers needed to supply their own.

This PR:

  1. Changes the definition file so that the variable supplies an example value instead of a default.
  2. Updates the create-pwa wizard accordingly

Related Issue

Closes PWA-84.

Acceptance

Verification Stakeholders

@zetlen @jcalcaben

Specification

Verification Steps

📝 Creating a .env file (new projects)

  1. Run npx buildpack create-env-file .
  2. See that you get the following output, asking you to supply a CHECKOUT_BRAINTREE_TOKEN (and providing you with an example value).
$ npx buildpack create-env-file .
  ⓧ  Missing required environment variables:
     MAGENTO_BACKEND_URL: Connect to an instance of Magento 2.3 by specifying its public domain name. (eg.
     "https://master-7rqtwti-mfwmkrjfqvbjk.us-4.magentosite.cloud/")
     CHECKOUT_BRAINTREE_TOKEN: Specify a Braintree API token token to direct the Venia storefront to communicate with your Braintree instance. You can find
     this value in Braintree's Control Panel under Settings > API Keys > Tokenization Keys. (eg. "sandbox_8yrzsvtm_s2bg8fs563crhqzk")
  ⚠  The current environment is not yet valid; please set any missing variables to build the project before generating a .env file.

📝 Modifying a .env file (existing projects)

➡️ Missing CHECKOUT_BRAINTREE_TOKEN
  1. Comment out or remove CHECKOUT_BRAINTREE_TOKEN from your project's .env file: #CHECKOUT_BRAINTREE_TOKEN=some_value
  2. yarn install
  3. Verify the following output error message:
Ensuring valid environment...
(node:12733) UnhandledPromiseRejectionWarning: Error: Command failed: pwa-studio/packages/pwa-buildpack/bin/buildpack load-env --core-dev-mode pwa-studio/packages/venia-concept
  ⓧ  Missing required environment variables:
     CHECKOUT_BRAINTREE_TOKEN: Specify a Braintree API
     token token to direct the Venia storefront to
     communicate with your Braintree instance. You can
     find this value in Braintree's Control Panel under
     Settings > API Keys > Tokenization Keys. (eg.
     "sandbox_8yrzsvtm_s2bg8fs563crhqzk")
➡️ Has CHECKOUT_BRAINTREE_TOKEN set to old default value
  1. Set CHECKOUT_BRAINTREE_TOKEN from your project's .env file to sandbox_8yrzsvtm_s2bg8fs563crhqzk: CHECKOUT_BRAINTREE_TOKEN=sandbox_8yrzsvtm_s2bg8fs563crhqzk
  2. yarn install
  3. Verify the following output warning message:
Ensuring valid environment...
  ⚠  Default value for CHECKOUT_BRAINTREE_TOKEN has changed in PWA Studio Buildpack v4.0.0, due to It was unclear that developers should provide their own Braintree tokens. An example value is provided instead.
     Old value: sandbox_8yrzsvtm_s2bg8fs563crhqzk
     New value: undefined
     This project is using the old default value for CHECKOUT_BRAINTREE_TOKEN. Check to make sure the change does not cause regressions.
➡️ Has CHECKOUT_BRAINTREE_TOKEN set to some other value
  1. Set CHECKOUT_BRAINTREE_TOKEN from your project's .env file to some_other_value: CHECKOUT_BRAINTREE_TOKEN=some_other_value
  2. yarn install
  3. Verify the following message:
Ensuring valid environment...
✨  Done in 2.46s.

📝 Creating a Project via the Create PWA Wizard

Navigate to the create-pwa package: cd packages/create-pwa.

➡️ Use the example value
  1. Run the wizard: node ./bin/create-pwa
  2. Verify the wizard asks for a Braintree token and supplies the example value
  3. Use the example value
  4. Finish the wizard
  5. Open your project's .env file (located at packages/create-pwa/<your project name>/.env)
  6. Verify that CHECKOUT_BRAINTREE_TOKEN is set: CHECKOUT_BRAINTREE_TOKEN=sandbox_8yrzsvtm_s2bg8fs563crhqzk.
  7. Verify that the following warning appeared in the console:
  ⚠  Default value for CHECKOUT_BRAINTREE_TOKEN has changed in PWA Studio Buildpack v4.0.0, due to confusion about whether developers should provide their own Braintree tokens for their own sites. An example value is provided instead for development purposes.
     Old value: sandbox_8yrzsvtm_s2bg8fs563crhqzk
     New value: undefined
     This project is using the old default value for CHECKOUT_BRAINTREE_TOKEN. Check to make sure the change does not cause regressions.
➡️ Supply your own value
  1. Run the wizard: node ./bin/create-pwa
  2. Answer my_custom_value to the braintree token question
  3. Finish the wizard
  4. Open your project's .env file (located at packages/create-pwa/<your project name>/.env)
  5. Verify that CHECKOUT_BRAINTREE_TOKEN is set: CHECKOUT_BRAINTREE_TOKEN=my_custom_value.
  6. Verify that no warning or error message related to CHECKOUT_BRAINTREE_TOKEN appeared in the console.
➡️ Set an environment variable first
  1. Run the wizard but with an environment variable: CHECKOUT_BRAINTREE_TOKEN=override node ./bin/create-pwa
  2. Open your new project's .env file and verify that CHECKOUT_BRAINTREE_TOKEN is set: CHECKOUT_BRAINTREE_TOKEN=override.
  3. Verify the following console warning appeared:
  ⚠  Command line option --braintree-token was set to 'sandbox_8yrzsvtm_s2bg8fs563crhqzk', but environment variable {"CHECKOUT_BRAINTREE_TOKEN":"override"} conflicts with it. Environment variable overrides!

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@supernova-at supernova-at added version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. version: Minor This changeset includes functionality added in a backwards compatible manner. and removed version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. labels Nov 27, 2019
@supernova-at
Copy link
Contributor Author

Also: please verify that this should be a MINOR change.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Nov 27, 2019

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-84.

Generated by 🚫 dangerJS against 666f011

sirugh
sirugh previously approved these changes Nov 27, 2019
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Nice! This should be much clearer to those getting started. Also, I think minor is correct for this change. "Backwards compatible, but new functionality".

@jcalcaben jcalcaben added docs documentation documentation This pertains to documentation. labels Dec 2, 2019
packages/pwa-buildpack/envVarDefinitions.json Show resolved Hide resolved
packages/pwa-buildpack/envVarDefinitions.json Outdated Show resolved Hide resolved
packages/pwa-buildpack/envVarDefinitions.json Outdated Show resolved Hide resolved
@tjwiebell
Copy link
Contributor

@supernova-at
Are we supposed to support the create-pwa flow as well? I had some issues simulating this w/o this being published, but after installing create-pwa locally, I was able to generate this buildpack command, which had to be run relative to pwa-studio directory:

yarn buildpack create-project create-pwa-test --template "venia-concept" --name "create-pwa-test" --author "Tommy Wiebell <[email protected]>" --backend-url "https://master-7rqtwti-mfwmkrjfqvbjk.us-4.magentosite.cloud/" --npm-client "yarn"

which successfully built the app, but did get this output:

  ℹ  Found venia-concept directory
  ℹ  Creating a new PWA project 'full-path-test' in /full-path-test
  ⓧ  Missing required environment variables:
     CHECKOUT_BRAINTREE_TOKEN: Specify a Braintree API token token to direct the Venia storefront to communicate with
     your Braintree instance. You can find this value in Braintree's Control Panel under Settings > API Keys >
     Tokenization Keys. (eg. "sandbox_8yrzsvtm_s2bg8fs563crhqzk")
  ⚠  The current environment is not yet valid; please set any missing variables to build the project before
     generating a .env file.
  ℹ  Successfully wrote a fresh configuration file to /Users/twiebell/Projects/full-path-test/.env

Should we also add a question to the wizard that asks for this token, or does that seem too early in the setup process? It does seem preferable not to error here, but unless the developer was paying close attention, this notice would get buried by yarn install output, and lead to the problem you're attempting to fix.

@supernova-at
Copy link
Contributor Author

supernova-at commented Dec 3, 2019

Are we supposed to support the create-pwa flow as well?

Great catch, yes - thanks! Updated the wizard in f0859e0.

I also updated the PR description and verification steps.

zetlen
zetlen previously approved these changes Dec 3, 2019
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Outstanding. Good catches and excellent work, everyone!

@sirugh
Copy link
Contributor

sirugh commented Dec 5, 2019

Error from AWS console:

Summary of all failing tests 
FAIL packages/pwa-buildpack/lib/__tests__/cli-load-env.spec.js 
  ● handler loads from dotenv file 
 
    expect(jest.fn()).not.toHaveBeenCalled() 
 
    Expected mock function not to be called but it was called with: 
      ["  ⚠  Environment variable BRAINTREE_TOKEN has been 
         renamed in PWA Studio Buildpack v4.0.0. Its new 
         name is CHECKOUT_BRAINTREE_TOKEN."] 
 
      66 |     // Assert. 
      67 |     expect(result).toBeUndefined(); 
    > 68 |     expect(console.warn).not.toHaveBeenCalled(); 
         |                              ^ 
      69 | }); 
      70 |  
      71 | test('warns if dotenv file does not exist', () => { 
 
      at Object.toHaveBeenCalled (packages/pwa-buildpack/lib/__tests__/cli-load-env.spec.js:68:30) 
 
 
Test Suites: 1 failed, 233 passed, 234 total 

@sirugh
Copy link
Contributor

sirugh commented Dec 5, 2019

You may need to do some work in pwa-studio-cicd for this to past tests :/

https://github.com/magento/pwa-studio-cicd/search?utf8=%E2%9C%93&q=BRAINTREE_TOKEN&type=

@tjwiebell tjwiebell merged commit a2dc69f into develop Dec 6, 2019
@tjwiebell tjwiebell deleted the supernova/84_braintree_token branch December 6, 2019 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs documentation documentation This pertains to documentation. pkg:create-pwa pkg:pwa-buildpack version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants