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.

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

Fix CDK errors not being logged #211

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Fix CDK errors not being logged #211

merged 3 commits into from
Nov 12, 2024

Conversation

corymhall
Copy link
Contributor

For some reason trying to log an error using error does not log
anything. Also, we were not throwing the error so we were continuing
until it failed later with a different very unhelpful error message.

Instead of logging (since it doesn't work) just throw the error

For some reason trying to log an error using `error` does not log
anything. Also, we were not throwing the error so we were continuing
until it failed later with a different very unhelpful error message.

Instead of logging (since it doesn't work) just throw the error
@corymhall corymhall self-assigned this Nov 12, 2024
@corymhall corymhall mentioned this pull request Nov 12, 2024
@@ -152,15 +151,13 @@ export class App
const message = e.message as string;
const messageParts = message.split('Context lookups have been disabled. ');
const missingParts = messageParts[1].split('Missing context keys: ');
error(
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

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

There is a control flow difference between before vs after. Before, the error command did not terminate the control flow. After, throw terminates the control flow. Which one we want here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to throw. These errors that are thrown by CDK are fatal errors that prevent synthesis from completing. If we don't terminate here we end up with a very unhelpful error about not being able to read the cloud assembly.

@corymhall corymhall requested a review from t0yv0 November 12, 2024 19:36
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

LGTM

@corymhall corymhall merged commit 5912f83 into main Nov 12, 2024
11 checks passed
@corymhall corymhall deleted the corymhall/fix-errors branch November 12, 2024 19:49
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v1.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants