Move test resource setup from Python to Typescript#2438
Move test resource setup from Python to Typescript#2438SylvainSenechal wants to merge 2 commits into
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
36a43c8 to
352b2fb
Compare
There was a problem hiding this comment.
this one is completely removed, was not used
| if (!env.DEPLOY_CRR_LOCATIONS) { | ||
| continue; | ||
| } | ||
| const accountName = location.name === env.CRR_DESTINATION_LOCATION_NAME |
There was a problem hiding this comment.
Non-null assertions on env.CRR_DESTINATION_ACCOUNT_NAME! and env.CRR_SOURCE_ACCOUNT_NAME! bypass type checking without runtime validation. If these env vars are missing while DEPLOY_CRR_LOCATIONS is true, accountsCreds[undefined] silently returns undefined, producing a confusing TypeError: Cannot read properties of undefined (reading 'AccessKeyId'). The original Python used os.environ[...] which raised a clear KeyError. Add a guard before using them.
| const accountName = location.name === env.CRR_DESTINATION_LOCATION_NAME | |
| if (!env.CRR_SOURCE_ACCOUNT_NAME || !env.CRR_DESTINATION_ACCOUNT_NAME) { | |
| throw new Error('CRR_SOURCE_ACCOUNT_NAME and CRR_DESTINATION_ACCOUNT_NAME are required when deploying CRR locations'); | |
| } | |
| const accountName = location.name === env.CRR_DESTINATION_LOCATION_NAME |
9f459d7 to
593dbfd
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
593dbfd to
22104b7
Compare
22104b7 to
f69ac66
Compare
| ENABLE_RING_TESTS: process.env.ENABLE_RING_TESTS?.toLowerCase() !== 'false', | ||
| DEPLOY_CRR_LOCATIONS: process.env.DEPLOY_CRR_LOCATIONS?.toLowerCase() !== 'false', | ||
| CRR_ROLE_NAME: process.env.CRR_ROLE_NAME ?? 'crr-role', | ||
| CRR_SOURCE_ACCOUNT_NAME: requireEnv('CRR_SOURCE_ACCOUNT_NAME'), |
There was a problem hiding this comment.
CRR_SOURCE_ACCOUNT_NAME, CRR_DESTINATION_ACCOUNT_NAME, and CRR_DESTINATION_LOCATION_NAME are eagerly required via requireEnv at startup, but they're only consumed in locations.ts inside the DEPLOY_CRR_LOCATIONS guard. The original Python accessed them lazily (os.environ['CRR_SOURCE_ACCOUNT_NAME'] only inside the CRR block), so setup succeeded with DEPLOY_CRR_LOCATIONS=false even when these vars were unset. This could break environments (e.g. codespaces) where CRR is disabled and these vars aren't configured. Consider making them optional like the cloud-provider credentials above.
Issue: ZENKO-5077
This change wasn't absolutely needed, but it was very cheap and easy to do with Claude, it goes toward aligning the codebase on a single programming language and a single typescript project, to ease future maintenance.
It also open doors for later progressive refactoring of test configuration, as the code is now shared, cucumber/mocha test could directly call functions defined in this configuration folder