-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: Add intrinsic support for AWS::Serverless::Api BasePath property #3808
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
base: develop
Are you sure you want to change the base?
Conversation
7efc3e8
to
d23f981
Compare
mapping_basepath = path if normalize_basepath else basepath | ||
logical_id = "{}{}{}".format(self.logical_id, path, "BasePathMapping") | ||
if is_intrinsic(basepath): | ||
mapping_basepath = basepath |
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.
So the parameter NormalizeBasePath
is not used anymore if the BasePath
is passed as an intrinsic?
logical_id = "{}{}{}".format(self.logical_id, path, "BasePathMapping") | ||
if is_intrinsic(basepath): | ||
mapping_basepath = basepath | ||
logical_id = self.logical_id + "BasePathMapping" |
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.
what happens if there are different base paths all defined with intrinsics? Will they just have the same logical id? Should we add some sort of hash for the intrinsic values here, so we actually create different resources?
@@ -58,7 +58,7 @@ Resources: | |||
DomainName: !Sub 'example-${AWS::Region}.com' | |||
CertificateArn: !Ref MyDomainCert | |||
EndpointConfiguration: !Ref EndpointConf | |||
BasePath: [/get, /fetch] | |||
BasePath: !Sub '${AWS::Region}-api' |
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.
Can you still keep the list, but do with intrinsics instead?
Something like
BasePath: !Sub '${AWS::Region}-api' | |
BasePath: | |
- !Sub '${AWS::Region}-get' | |
- !Sub '${AWS::Region}-fetch' |
basepath_value = self.domain.get("BasePath") | ||
# Create BasepathMappings | ||
if self.domain.get("BasePath") and isinstance(basepath_value, str): | ||
if isinstance(basepath_value, str) or is_intrinsic(basepath_value): |
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.
Thoughts on adding this same support on http_api_generator.py file?
Issue #, if available
N/a
Description of changes
The
BasePath
attribute ofAws::Serverless::Api
does not currently accept!Sub
as a parameter, instead leaving it as blank or returningNone
. As an example,Transforms to the below, without the actual BasePath being added.
After updates
With the updates to allow intrinsics for this property, the output now looks like:
Description of how you validated changes
Manually ran the transform with and without the updated changes and compared the templates.
Updated the transform tests to include this new case. I didn't add any new tests because there are other locations that have
BasePath
without the intrinsics, so I figured it was fine to add this in theapi_with_basic_custom_domain_intrinsics
tests.Also confirmed that the added tests failed with the older transform version.
Checklist
Examples?
Please reach out in the comments if you want to add an example. Examples will be
added to
sam init
through aws/aws-sam-cli-app-templates.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.