Context providers: return dummy values if env is undefined, but emit errors to fail "cdk synth" #227
Conversation
| @@ -69,8 +80,7 @@ export class ContextProvider { | |||
| const region = stack.env.region; | |||
|
|
|||
| if (account == null || region == null) { | |||
| // tslint:disable-next-line:max-line-length | |||
| throw new Error(`${providerDescription}: requires account and region information, but ${stack.name} doesn't have an "env" defined`); | |||
There was a problem hiding this comment.
I'm not quite comfortable with this. How are we still guaranteeing that account and region information are available when invoking a context provider "for reals"?
There was a problem hiding this comment.
In the realz worldz, a stack must be associated with an environment, either explicitly or implicitly. I don't think there's a use case for a stack without an env, is there?
There was a problem hiding this comment.
As long as you don't use a context provider there's no reason to require it, but we could.
But what's the mechanism that's making sure this is true, once you remove this error?
There was a problem hiding this comment.
Let me get back to you 😉
There was a problem hiding this comment.
@RomainMuller Is there a case where the toolkit invokes a CDK program with an undefined default environment (which is what "undefined env" translates to in the real world)?
There was a problem hiding this comment.
I don't believe there's any case where it does so intentionally (meaning besides those cases when attempts to figure out a default environment failed).
There was a problem hiding this comment.
As suggested by @rix0rrr, we decided that what we want is that the toolkit will fail synthesis with a meaningful error, but not through an exception but through error emitted as metadata during synthesis (similar to the warnings we have today). We will also add "info" messages, which will allow construct authors "communicate" information to users.
Throwing if "env" is undefined makes it hard to unit test stacks that use context providers. It requires dummy values for account and region, which is boilerplate and non-intuitive. Since we already have a concept of dummy context values, this change short-circuits context resolution and returns the dummy values in case the stack's env is undefined.
eladb
force-pushed
the
benisrae/dummy-context
branch
from
8823eac
to
8833955
Compare
The `bucketUrl` returns the URL of the bucket and `urlForObject(key)` returns the URL of an object within the bucket. Furthermore: `iam.IIdentityResource` was soft-renamed to `iam.IPrincipal` (IIdentityResource is still supported).
Assets represent local files or directories which can be bundled as part of CDK constructs. During deployment, the toolkit will upload assets to the "Toolkit Bucket", and use CloudFormation Parameters to reference the asset in deploy-time. Assets expose the following deploy-time attributes: * s3BucketName * s3ObjectKey * s3Url Furthermore, the `asset.grantRead(principal)` will add IAM read permissions for the asset to the desired principal.
…olved Instead of throwing an error when context cannot be resolved, we return dummy values but attach a metadata entry "aws:cdk:error" to the construct with a message explaining that context cannot be resolved. The toolkit picks up these metadata entries and will fail synthesis, unless --ignore-errors is set. The benefit is that this enables unit test use cases, where users are okay with dummy contextual information. INFO messages can also be emitted now using `context.addInfo(message)`. The metadata keys for info, warning and errors are normalized to use the "aws:cdk" prefix, and formally defined in cxapi.
eladb
changed the title
packages/aws-cdk/bin/cdk.ts
Outdated
| } | ||
|
|
||
| function printMessage(logFn: (s: string) => void, prefix: string, stackName: string, id: string, entry: cxapi.MetadataEntry) { | ||
| logFn(`${prefix}: ${entry.data} (at ${stackName}:${id})`); |
There was a problem hiding this comment.
What does this look like? Maybe better if turned around:
[${prefix} at ${stackName}:${id}] ${entry.data}
There was a problem hiding this comment.
Done. Here's how this looks now:
[Error at /app-with-vpc/MyVpc] Cannot determine scope for context provider availability-zones.
This usually happens when AWS credentials are not available and the default account/region cannot be determined.
There was a problem hiding this comment.
Yeah that may well be better :) Front-loading the actionable information :P
|
My cdk synth does not fail, despite:
From reading this PR summary, this should fail What am I missing? |
Throwing if "env" is undefined makes it hard to unit test stacks that
use context providers. It requires dummy values for account and region,
which is boilerplate and non-intuitive.
Since we already have a concept of dummy context values, this change
short-circuits context resolution and returns the dummy values in case
the stack's env is undefined.