-
Notifications
You must be signed in to change notification settings - Fork 123
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
Use the configured AWS partition when constructing ARNs #1223
Conversation
|
||
// sns | ||
// | ||
// SNS platform applications can be imported using the ARN: | ||
// arn:aws:sns:us-west-2:0123456789012:app/GCM/gcm_application | ||
"aws_sns_platform_application": config.TemplatedStringAsIdentifier("name", "arn:aws:sns:{{ .setup.configuration.region }}:{{ .setup.client_metadata.account_id }}:app/GCM/{{ .external_name }}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out hardcoding GCM in here only works for platform applications using Google cloud messaging. Platform applications using Apple Push Notification Service use APNS. This is simply the {{ .parameters.platform }}
. I'll open a separate PR for that change.
Signed-off-by: Matt Bush <[email protected]>
…tition Signed-off-by: Matt Bush <[email protected]>
Signed-off-by: Matt Bush <[email protected]>
Signed-off-by: Matt Bush <[email protected]>
Signed-off-by: Matt Bush <[email protected]>
Signed-off-by: Matt Bush <[email protected]>
} | ||
return cfg, nil | ||
} | ||
|
||
func getIAMRegion(pc *v1beta1.ProviderConfig) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should write a unit test for this function.
This provider repo does not have enough maintainers to address every pull request. Since there has been no activity in the last 90 days it is now marked as |
This pull request is being closed since there has been no activity for 14 days since marking it as |
@mbbush wonder if we can bring this in - tested today in gov region - looks like a working solution |
Thanks @haarchri we could definitely use this fix. |
Thanks everyone. This fix would be a life saver. |
Is this being prioritized? This is blocking us as well, using GovCloud. |
/fresh |
Yes, it has been prioritized, and if no blockers are found, we plan to include it in the next release. |
Just curious, when would the next release happen? |
It is scheduled for Thursday next week. |
Description of your changes
Stores the existing partition id from the provider config into a terraform configuration object, which is available when resolving external names.
Uses the partition as part of the external name config for the resources that currently hardcode a partition of
aws
in their templated ARN.Refactors the external name configurations which create arns to use simple helper functions, to reduce code repetition.
The default value of the partition is "aws", which is currently the only supported value, so this should be a nonbreaking change for users.
Fixes #881
Fixes #820
Addresses #855 (although probably not completely)
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested