fix(toolkit): handle isCn when region is not initialized to fix extension activation failure #7599
Conversation
…sion activation failure
|
| return getComputeRegion()?.startsWith('cn') ?? false | ||
| } catch (e) { | ||
| // If compute region isn't initialized yet, assume we're not in a CN region | ||
| getLogger().debug('isCn called before compute region initialized, defaulting to false') |
There was a problem hiding this comment.
should we also log the error message?
There was a problem hiding this comment.
Thanks for the review, added error logging as well as unit tests
There was a problem hiding this comment.
2 of the CI workflow checks are failing seemingly due to a change merged recently and not due to this commit, reference: https://github.com/aws/aws-toolkit-vscode/actions/runs/16175639516/job/45659998532
| // Restore original getComputeRegion if it was stubbed | ||
| if (utils.getComputeRegion.restore) { | ||
| utils.getComputeRegion.restore() | ||
| } |
There was a problem hiding this comment.
Do we need this? I see sandbox.restore() is called in our afterEach, doesn't this already reset all stubs?
There was a problem hiding this comment.
Good catch, not needed necessarily. Added it initially for best practice, but since sandbox is restored in afterEach, underlying stubs should also get cleaned
| } catch (err) { | ||
| getLogger().error(`Error in isCn method: ${err}`) | ||
| return false | ||
| } |
There was a problem hiding this comment.
Should we instead be throwing the error here? We already handle the known case above by returning false when the region is uninitialized, but this can catch other errors that I'm not sure we should assume returning false for.
There was a problem hiding this comment.
Good point, the case that throws error is handled already, and i didnt find any error cases that could result from the getComputeRegion method, hence it looks safe to not throw error back
|
/retryBuilds |
4 participants
Problem
With updating Toolkit version to latest in SMUS CodeEditor Spaces, observing Toolkit throws an error as attached in screenshot below with error message
Solution
isCn()resilient to uninitialized state and return a default valuefeature/xbranches will not be squash-merged at release time.