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

Add ability to specify resource options at the stack level #245

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented Nov 22, 2024

It has been possible to specify Pulumi resource options at the stack level, but it did not flow through to the actual resources. This PR makes sure that the inheritance works correctly.

This PR also adds functionality to automatically set the Stack environment based on the App provider. Because the App creates the stacks in an async context, we can use provider functions to lookup the environment and then pass the resolved environment to the stack.

For now this is something that the user will have to specify (example in the lookups example test). This would mean that all Stacks have their environment provided by default. This will cut down on the number of Intrinsics used in the generated template. The reason I am not turning this on by default is because it requires lookups to be enabled (environment agnostic stacks use Intrinsics to find availability zones, but explicit environment stacks use lookups).

If the user provides a provider to the Stack we are no longer in an async context which means we can't determine the environment from the provider and fall back to an environment agnostic stack.

re #61, re #219

}

if (nativeAccount !== awsAccount) {
warn(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a warning instead of throwing an error because it's possible this could still be correct.

* @param s - The string that contains the placeholders to replace
* @returns The string with the placeholders fully resolved
*/
protected resolvePlaceholders(s: string): Promise<string> {
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 was left over from before and it doesn't look like it is used anywhere anymore.

@@ -564,15 +567,15 @@ export class StackConverter extends ArtifactConverter implements intrinsics.Intr

switch (target) {
case 'AWS::AccountId':
return getAccountId({ parent: this.app.component }).then((r) => r.accountId);
return getAccountId({ parent: this.stackResource }).then((r) => r.accountId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parent should be the stack so that we use the provider associated with the stack.

It has been possible to specify Pulumi resource options at the stack
level, but it did not flow through to the actual resources. This PR
makes sure that the inheritance works correctly.

This PR also adds functionality to automatically set the Stack
environment based on the App provider. Because the App creates the
stacks in an async context, we can use provider functions to lookup the
environment and then pass the resolved environment to the stack. This
means that all Stacks have their environment provided by default. This
will cut down on the number of Intrinsics used in the generated
template.

If the user provides a provider to the Stack we are no longer in an
async context which means we can't determine the environment from the
provider and fall back to an environment agnostic stack.

re #61, re #219
@corymhall
Copy link
Contributor Author

useful in cases where you need to deploy resources to different AWS regions.

```ts
const awsProvider = new aws.Provider('aws-provider');
Copy link
Member

Choose a reason for hiding this comment

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

examples are better with imports I think, if not too hard. To see where aws is coming from.

One thing to note is that when you pass different custom providers to a Stack,
by default the Stack becomes an [environment agnostic stack](https://docs.aws.amazon.com/cdk/v2/guide/configure-env.html#configure-env-examples).
If you want to have the environment specified at the CDK Stack level, then you
also need to provide the environment to the Stack Props.
Copy link
Member

Choose a reason for hiding this comment

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

Stack Props, as follows:

const east2Stack = new pulumicdk.Stack('east2-stack', {
props: {
env: {
region: 'us-east-2',
Copy link
Member

Choose a reason for hiding this comment

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

I think I understand now the comment you had regarding not being able to pass a pulumi.Output region in here automatically.

Copy link
Member

Choose a reason for hiding this comment

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

I was looking at IResolvable but it's unclear that that's allowed to return promises to CDK.

@@ -150,6 +179,17 @@ export class App
this.appOptions = props.args;
const lookupsEnabled = process.env.PULUMI_CDK_EXPERIMENTAL_LOOKUPS === 'true';
const lookups = lookupsEnabled && pulumi.runtime.isDryRun();
const account = await native
Copy link
Member

Choose a reason for hiding this comment

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

Micro-optimization: you can do these in parallel.

@@ -230,6 +295,8 @@ export interface StackOptions extends pulumi.ComponentResourceOptions {
props?: cdk.StackProps;
}

type Writeable<T> = { -readonly [P in keyof T]: T[P] };
Copy link
Member

Choose a reason for hiding this comment

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

Still used? Linter says unused.

@@ -336,6 +477,22 @@ function generateAppId(): string {
.slice(-17);
}

function hasProvider(
Copy link
Member

Choose a reason for hiding this comment

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

Linter thinks unused.

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.

Seems like a useful feature, thank you!

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.

2 participants