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 getRegion functions #1384

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Conversation

flostadler
Copy link
Contributor

The getRegion functions were incorrectly trying to extract the current region from the provider. This silently resolved to undefined in certain cases; e.g. when no AWS_REGION ENV variable was set.
In particular this caused ECS tasks to fail because they used https://logs.undefined.amazonaws.com/ as the endpoint for cloudwatch.

Instead we should use the getRegion data source to correctly identify the current region.

Fixes #1112
Fixes #1279

@flostadler flostadler requested review from t0yv0, corymhall and a team September 25, 2024 20:30
@flostadler flostadler self-assigned this Sep 25, 2024
awsx/utils.ts Outdated
const provider = res.getProvider ? res.getProvider("aws::") : undefined;
return getRegionFromProvider(provider);
// uses the provider from the parent resource to fetch the region
return aws.getRegionOutput({}, { parent: res }).apply(region => region.name as aws.Region);
Copy link
Member

Choose a reason for hiding this comment

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

To confirm, as this inheritance is a little tricky, if res uses a custom version of the AWS provider, { parent: res} will inherit that custom version so we get the right thing 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.

Yes, that's my understanding. It actually retrieves the provider configured for the parent. If that one's using a custom version then this will also use that version.

Copy link
Member

Choose a reason for hiding this comment

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

I see this is already shipped in aws-apigateway so that's nice! THank you.

@@ -16,7 +16,7 @@ import * as aws from "@pulumi/aws";
import * as pulumi from "@pulumi/pulumi";
import { ResourceOptions } from "@pulumi/pulumi";
import * as schema from "../schema-types";
import { getRegion, getRegionFromOpts, parseArn } from "../utils";
Copy link
Member

Choose a reason for hiding this comment

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

Cleanup? Some missing liner rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm currently trying to see which one we're missing

awsx/utils.ts Outdated
const provider = res.getProvider ? res.getProvider("aws::") : undefined;
return getRegionFromProvider(provider);
// uses the provider from the parent resource to fetch the region
return aws.getRegionOutput({}, { parent: res }).apply(region => region.name as aws.Region);
}

function getRegionFromProvider(provider: pulumi.ProviderResource | undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Specify return type here please. I wonder what the "default" value was, aws.config.region. Now with this change it will pull the ambient AWS provider if provider=undefined, sounds like it's intended.

@t0yv0 t0yv0 self-requested a review September 25, 2024 20:43
@flostadler flostadler merged commit a12ec3b into master Sep 25, 2024
11 checks passed
@flostadler flostadler deleted the flostadler/regress-1112-ecs-region branch September 25, 2024 23:23
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v2.16.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
3 participants