Catalyst 1744 refactor configuration variables#2866
Catalyst 1744 refactor configuration variables#2866jamesqquick wants to merge 16 commits intocanaryfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
fc5af19 to
cd4009b
Compare
Co-authored-by: Chancellor Clark <chancellor.clark@bigcommerce.com>
…and define priority for config variables
… and read all variables from config file
84dcd1a to
6091753
Compare
matthewvolk
left a comment
There was a problem hiding this comment.
.bigcommerce/, the root .gitignore contains a line to ignore .bigcommerce but the core/.gitignore does not. Since eventually, core/.gitignore will be the ignore file shipped to customers, and they won't have the root ignore file, can we please add .bigcommerce to core/.gitignore?
| const envFileIndex = process.argv.findIndex( | ||
| (arg) => arg === '--env-file' || arg.startsWith('--env-file='), | ||
| ); |
There was a problem hiding this comment.
This works but I think it's fragile... I'm not sure it'll handle edge cases like --env-file being passed after -- (which Commander uses to stop option parsing).
If we're ok with that, that's fine. We can always merge and re-evaluate if it starts behaving undesirably.
| consolaPromptMock.mockRestore(); | ||
| }); | ||
|
|
||
| test('prompts to create a new project', async () => { |
There was a problem hiding this comment.
I think this test name is a duplicate of the test on line 164 in this same file. I see the following:
- line 164:
test('prompts to create a new project', ...)- the happy path where the user selects "create" and it succeeds - line ~221:
test('prompts to create a new project', ...)- the error path where the API returns a 502
Maybe we rename the second one to "errors when project creation fails" or something?
| 'BIGCOMMERCE_STORE_HASH=123', | ||
| 'BIGCOMMERCE_STOREFRONT_TOKEN=456', | ||
| 'CATALYST_STORE_HASH=123', | ||
| 'CATALYST_ACCESS_TOKEN=456', |
There was a problem hiding this comment.
Was this intentional, to rename BIGCOMMERCE_STOREFRONT_TOKEN to CATALYST_ACCESS_TOKEN?
| { | ||
| type: 'secret', | ||
| key: 'BIGCOMMERCE_STOREFRONT_TOKEN', | ||
| key: 'CATALYST_ACCESS_TOKEN', |
There was a problem hiding this comment.
Was this intentional, to rename BIGCOMMERCE_STOREFRONT_TOKEN to CATALYST_ACCESS_TOKEN?
There was a problem hiding this comment.
I actually just realized I made that jump. I think I just had access_token in my mind and put that in the different docs, then copied in. I don't think we made a collective decision on that. I think the correct thing to do would be CATALYST_STOREFRONT_TOKEN. Does you agree with that?
There was a problem hiding this comment.
Wait ignore the above lol I get it now. This is supposed to be testing parsing secret runtime variables. Great catch!
chanceaclark
left a comment
There was a problem hiding this comment.
Can we split this up into separate changes for each thing you are doing here?
Into separate PRs you mean? I wondered about it being too much in one. I can do that! |
|
Splitting this into separate PRs |
What/Why?
Refactor config variables:
Rename environment variables from
BIGCOMMERCE_prefix toCATALYST_prefixBIGCOMMERCE_STORE_HASH->CATALYST_STORE_HASHBIGCOMMERCE_PROJECT_UUID->CATALYST_PROJECT_UUIDBIGCOMMERCE_FRAMEWORK->CATALYST_FRAMEWORKBIGCOMMERCE_ACCESS_TOKEN- >CATALYST_ACCESS_TOKENImplement new priority for config variables
Add --env-file flag
This allows the user to specify a environment variable file to load variables from
Remove default environment variable file
Remove the reading of default environment variable files. This requires the user to pass
--file-envif they want variables to be read from a file.Expand project.json
Write additional config variables to project.json file.
projectUuidframeworkstoreHashaccessTokenImproved logging
Update logs to details which variables are required and how/where to set them
Testing
Tests have been added to test basic functionality. One thing the tests do not cover is when the user is passing the
--env-fileflag to specify an environment variable file to read from. For testing,CATALYST_prefixproject list) without passing any flags. It should fail since no env file is being picked up automaticallycatalyst project createcommand to generate a new project and verify the correct properties are set inproject.jsoncatalyst project list- should now work by reading variables fromproject.jsoncatalyst project list --access-token dummy-tokenand pass dummy variable for access token via--access-tokenflag. This should fail since the explicitly passed access token isn't valid.env.testwith invalid access token. Then, runcatalyst project list --env-file .env.testand show that the command fails since it picked up the invalid access tokenMigration