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.

Sign up for GitHub

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

feat(packages/twilio-run): regionalize toolkit config and api #433

Merged
merged 6 commits into from
Nov 7, 2022
Merged

feat(packages/twilio-run): regionalize toolkit config and api #433

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fb54415
feat(packages/twilio-run): regionalize the .twiliodeployinfo
mbeldartwilio Oct 28, 2022
290ca58
feat: regional support for config files
mbeldartwilio Nov 1, 2022
e208a4a
chore(runtime-handler/packager.json): override a check
mbeldartwilio Nov 1, 2022
4f9f302
chore: typescript override for lodash types
mbeldartwilio Nov 2, 2022
e429bd5
refactor: using optional chaining
mbeldartwilio Nov 4, 2022
e44391e
fix: passing missing region param to related functions
mbeldartwilio Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/runtime-handler/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@
},
"overrides": {
"@types/prettier": "2.6.0",
"@types/express-serve-static-core": "ts3.9"
"@types/express-serve-static-core": "ts3.9",
"@types/lodash": "ts3.9"
},
"gitHead": "6db273648ed19474f4125042556b10c051529912"
}
14 changes: 14 additions & 0 deletions packages/serverless-api/src/api/utils/__tests__/api-client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,18 @@ describe('getApiUrl', () => {
const url = getApiUrl(DEFAULT_TEST_CLIENT_CONFIG);
expect(url).toBe('https://serverless.sydney.au2.twilio.com/v1');
});

test('handles edge with only eligible region with variable', () => {
process.env.TWILIO_REGION = 'au1';
const url = getApiUrl(DEFAULT_TEST_CLIENT_CONFIG);
expect(url).toBe('https://serverless.sydney.au1.twilio.com/v1');
});

test('handles edge with only eligible region with params', () => {
const url = getApiUrl({
...DEFAULT_TEST_CLIENT_CONFIG,
region: 'us1',
});
expect(url).toBe('https://serverless.ashburn.us1.twilio.com/v1');
});
});
17 changes: 16 additions & 1 deletion packages/serverless-api/src/api/utils/api-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
import { ClientConfig } from '../../types';

const regionEdgeMap: { [index: string]: string } = {
us1: 'ashburn',
au1: 'sydney',
ie1: 'dublin',
'stage-us1': 'ashburn',
'stage-au1': 'sydney',
};

export function getApiUrl(
config: ClientConfig,
product = 'serverless',
apiVersion = 'v1'
): string {
const configEdge = config.edge || process.env.TWILIO_EDGE;
const configRegion = config.region || process.env.TWILIO_REGION;
const region = configRegion ? `${configRegion}.` : '';

if (!configEdge && configRegion) {
const defaultEdge = regionEdgeMap[configRegion]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember Dom mentioning, we should let toolkit fallback to the API and be dumb about this.

Copy link

@vingiarrusso vingiarrusso Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hm. Yes, but in this case, it's not exactly expected or desired behavior imo. If i recall the conversation correctly, we discussed if the user specifies an edge we don't have, and then we try to use that edge in the URL, then they'd get a 404 back which isn't great or specific, but at least it would fail. In this case, you specify an invalid region and end up with something deployed to US1, which is kind of unexpected. @dkundel thoughts?

edit: misread what this did

Copy link

Choose a reason for hiding this comment

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

oh wait, im totally misreading! my fault. That looks like what this will do. Okay all good :)

? `${regionEdgeMap[configRegion]}.`
: '';
return `https://${product}.${defaultEdge}${region}twilio.com/${apiVersion}`;
}

const edge = configEdge ? `${configEdge}.` : '';
const region = configRegion ? `${configRegion}.` : '';
return `https://${product}.${edge}${region}twilio.com/${apiVersion}`;
}
65 changes: 65 additions & 0 deletions packages/twilio-run/__tests__/config/global.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,69 @@ describe('readSpecializedConfig', () => {
env: '.env.stage',
});
});

test('account + region config override', () => {
__setTestConfig({
serviceSid: 'ZS11112222111122221111222211112222',
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what I understand, the ZS...22 is default servicesid. Other service sids under "envs" and "accounts" supposed to override the default one.

env: '.env.example',
commands: {
deploy: {
functionsFolder: '/tmp/functions',
},
},
environments: {
prod: {
serviceSid: 'ZS11112222111122221111222211112223',
env: '.env.prod',
},
},
projects: {
'AC11112222111122221111222211114444:au1': {
serviceSid: 'ZS11112222111122221111222211114444',
},
'AC11112222111122221111222211114444:ie1': {
serviceSid: 'ZS11112222111122221111222211114445',
},
AC11112222111122221111222211114444: {
serviceSid: 'ZS11112222111122221111222211114446',
},
},
});

expect(
readSpecializedConfig('/tmp', '.twilioserverlessrc', 'deploy', {
environmentSuffix: 'prod',
username: 'AC11112222111122221111222211114444',
region: 'ie1',
})
).toEqual({
serviceSid: 'ZS11112222111122221111222211114445',
functionsFolder: '/tmp/functions',
env: '.env.prod',
});

expect(
readSpecializedConfig('/tmp', '.twilioserverlessrc', 'deploy', {
environmentSuffix: 'prod',
username: 'AC11112222111122221111222211114444',
region: 'au1',
})
).toEqual({
serviceSid: 'ZS11112222111122221111222211114444',
functionsFolder: '/tmp/functions',
env: '.env.prod',
});

expect(
readSpecializedConfig('/tmp', '.twilioserverlessrc', 'deploy', {
environmentSuffix: 'prod',
username: 'AC11112222111122221111222211114444',
region: 'us1',
})
).toEqual({
serviceSid: 'ZS11112222111122221111222211114446',
functionsFolder: '/tmp/functions',
env: '.env.prod',
});
});
});
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
Copy link
Member

Choose a reason for hiding this comment

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

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.

});

flags = mergeFlagsAndConfig<DeployCliFlags>(configFlags, flags, cliInfo);
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/env/env-get.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<EnvGetFlags>(configFlags, flags, cliInfo);
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/env/env-import.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<EnvImportFlags>(configFlags, flags, cliInfo);
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/env/env-list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<EnvListFlags>(configFlags, flags, cliInfo);
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/env/env-set.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<EnvSetFlags>(configFlags, flags, cliInfo);
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/env/env-unset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<EnvUnsetFlags>(configFlags, flags, cliInfo);
Expand Down
19 changes: 14 additions & 5 deletions packages/twilio-run/src/config/global.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { getConfig } from './utils/configLoader';
export type SpecializedConfigOptions = {
username: string;
environmentSuffix: string;
region: string;
};

export const EXCLUDED_FLAGS = [
Expand Down Expand Up @@ -68,12 +69,20 @@ export function readSpecializedConfig<T extends CommandConfigurationNames>(
opts &&
opts.username &&
projectsConfig &&
projectsConfig[opts.username]
(projectsConfig[opts.username] ||
projectsConfig[`${opts.username}:${opts.region}`])
) {
result = {
...result,
...projectsConfig[opts.username],
};
if (projectsConfig[`${opts.username}:${opts.region}`]) {
result = {
...result,
...projectsConfig[`${opts.username}:${opts.region}`],
};
} else if (opts.region === 'us1' || opts.region === undefined) {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

result = {
...result,
...projectsConfig[opts.username],
};
}
}

EXCLUDED_FLAGS.forEach((key) => {
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<ListCliFlags>(configFlags, flags, cliInfo);
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/logs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<LogsCliFlags>(configFlags, flags, cliInfo);
Expand Down
1 change: 1 addition & 0 deletions packages/twilio-run/src/config/promote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export async function getConfigFromFlags(
(externalCliOptions && externalCliOptions.accountSid) ||
undefined,
environmentSuffix: flags.environment,
region: flags.region,
});

flags = mergeFlagsAndConfig<PromoteCliFlags>(configFlags, flags, cliInfo);
Expand Down
22 changes: 19 additions & 3 deletions packages/twilio-run/src/serverless-api/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ export async function getFunctionServiceSid(
cwd: string,
configName: string,
commandConfig: 'deploy' | 'list' | 'logs' | 'promote' | 'env',
username?: string
username?: string,
region?: string
): Promise<string | undefined> {
const twilioConfig = readSpecializedConfig(cwd, configName, commandConfig, {
username,
region,
});
if (twilioConfig.serviceSid) {
debug('Found serviceSid in config, "%s"', twilioConfig.serviceSid);
Expand All @@ -35,6 +37,18 @@ export async function getFunctionServiceSid(
if (username) {
debug('Attempting to read serviceSid from a deployinfo file');
const deployInfoCache = getDeployInfoCache(cwd);
if (
deployInfoCache &&
deployInfoCache[`${username}:${region}`] &&
deployInfoCache[`${username}:${region}`].serviceSid
Copy link

@vingiarrusso vingiarrusso Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

can we use optional chaining here? if (deployInfoCache[`${username}:${region}`]?.serviceSid) ...

Copy link

Choose a reason for hiding this comment

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

You should be able to since the node version is >14.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, this project should support node12 and above (from readme)
I would like to know Dom's opinion as node12 is deprecated already.

Copy link
Member

Choose a reason for hiding this comment

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

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

) {
debug(
'Found service sid by region from deploy info, "%s"',
deployInfoCache[`${username}:${region}`].serviceSid
);
return deployInfoCache[`${username}:${region}`].serviceSid;
}

if (
deployInfoCache &&
deployInfoCache[username] &&
Expand All @@ -49,20 +63,22 @@ export async function getFunctionServiceSid(
}

debug('Could not determine existing serviceSid');
debug(`${username}:${region}`);
return undefined;
}

export async function saveLatestDeploymentData(
Copy link
Member

Choose a reason for hiding this comment

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

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.

cwd: string,
serviceSid: string,
buildSid: string,
username?: string
username?: string,
region?: string
): Promise<void> {
if (!username) {
return;
}

return updateDeployInfoCache(cwd, username, {
return updateDeployInfoCache(cwd, username, region, {
serviceSid,
latestBuild: buildSid,
});
Expand Down
9 changes: 8 additions & 1 deletion packages/twilio-run/src/utils/deployInfoCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export function getDeployInfoCache(
export function updateDeployInfoCache(
baseDir: string,
username: string,
region: string = 'us1',
deployInfo: DeployInfo,
deployInfoCacheFileName: string = '.twiliodeployinfo'
): void {
Expand All @@ -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');
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

debug(`${username}:${region}`);
delete currentDeployInfoCache[username];
}

const newDeployInfoCache = {
...currentDeployInfoCache,
[username]: deployInfo,
[`${username}:${region}`]: deployInfo,
};

if (!validDeployInfoCache(newDeployInfoCache)) {
Expand Down