feat(packages/twilio-run): regionalize toolkit config and api #433
Conversation
mbeldartwilio
changed the title
mbeldartwilio
changed the title
mbeldartwilio
changed the title
| const region = configRegion ? `${configRegion}.` : ''; | ||
|
|
||
| if (!configEdge && configRegion) { | ||
| const defaultEdge = regionEdgeMap[configRegion] |
There was a problem hiding this comment.
I remember Dom mentioning, we should let toolkit fallback to the API and be dumb about this.
| if ( | ||
| deployInfoCache && | ||
| deployInfoCache[`${username}:${region}`] && | ||
| deployInfoCache[`${username}:${region}`].serviceSid |
There was a problem hiding this comment.
can we use optional chaining here? if (deployInfoCache[`${username}:${region}`]?.serviceSid) ...
There was a problem hiding this comment.
You should be able to since the node version is >14.
There was a problem hiding this comment.
Ideally, this project should support node12 and above (from readme)
I would like to know Dom's opinion as node12 is deprecated already.
There was a problem hiding this comment.
I'm okay with moving to Node 14 now. I think that's outdated in the README since twilio-run itself already only supports Node 14 since a few versions ago
| ...result, | ||
| ...projectsConfig[`${opts.username}:${opts.region}`], | ||
| }; | ||
| } else if (opts.region === 'us1' || opts.region === undefined) { |
There was a problem hiding this comment.
do we need to put this first? not sure if we're allowing username:us1, but it seems like the opts.region === 'us1' case would fall into the if block, so we should remove it or swap the if and else if blocks.
There was a problem hiding this comment.
I think logic you mentioned would fail in following case:
region=us1 and the config file with account:us1.
And yes, we will be allowing accountsid:us1
| @@ -71,9 +72,15 @@ export function updateDeployInfoCache( | |||
| deployInfoCacheFileName | |||
| ); | |||
|
|
|||
| if (currentDeployInfoCache.hasOwnProperty(username) && region === 'us1') { | |||
| debug('Invalid format for deploy info key. Overriding with region us1'); | |||
There was a problem hiding this comment.
i'm confused about this block, is the username always an account sid? if so, it seems like this would only be invalid if just the account sid key is in the cache, but the region is not us1? lmk if i'm misunderstanding though.
There was a problem hiding this comment.
this is handling the case where user has passed us1 in new build request but the existing config is still using 'username' and not 'username:us1'.
We wan't to remove 'username' as we are adding 'username:us1'. (see next few lines)
|
|
||
| test('account + region config override', () => { | ||
| __setTestConfig({ | ||
| serviceSid: 'ZS11112222111122221111222211112222', |
There was a problem hiding this comment.
the sid convention is a little confusing here, is this service sid supposed to match up with any of the ones below? or does only the service sids in projects matter for this test?
There was a problem hiding this comment.
from what I understand, the ZS...22 is default servicesid. Other service sids under "envs" and "accounts" supposed to override the default one.
| @@ -49,20 +55,22 @@ export async function getFunctionServiceSid( | |||
| } | |||
|
|
|||
| debug('Could not determine existing serviceSid'); | |||
| debug(`${username}:${region}`); | |||
| return undefined; | |||
| } | |||
|
|
|||
| export async function saveLatestDeploymentData( | |||
There was a problem hiding this comment.
It looks like you are missing in the packages/twilio-run/src/commands/deploy.ts file an update to the call of saveLatestDeploymentData that passes config.region.
| @@ -74,6 +74,7 @@ export async function getConfigFromFlags( | |||
| (externalCliOptions && externalCliOptions.accountSid) || | |||
| undefined, | |||
| environmentSuffix: flags.environment, | |||
| region: flags.region, | |||
There was a problem hiding this comment.
The calls to getFunctionServiceSid are missing the region passed in as the cache from the twilioDeployInfo file are not being read. Same applies in the other files.
Contributing to Twilio