-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(amplify-domain): Added config for auto subdomain creation #13342
feat(amplify-domain): Added config for auto subdomain creation #13342
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
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.
Some small changes. Additionally one of the tests fails to compile because, but I believe this will be fixed by making enabledAutoSubdomain
optional.
Pull request has been modified.
9a4b40d
to
4959332
Compare
4959332
to
19ac74d
Compare
@MrArnoldPalmer Review suggestion done, as well as lint, test, doc and build fixes. Please, give it another review ;-) |
@@ -106,6 +127,9 @@ export class Domain extends Resource { | |||
appId: props.app.appId, | |||
domainName, | |||
subDomainSettings: Lazy.any({ produce: () => this.renderSubDomainSettings() }, { omitEmptyArray: true }), | |||
enableAutoSubDomain: !!props.enableAutoSubdomain, | |||
autoSubDomainCreationPatterns: props.autoSubdomainCreationPatterns || ['*', 'pr*'], |
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.
This makes it so that only when a user passes enableAutoSubdomain: true
all branches will automatically get their own subdomains deployed right? if enableAutoSubDomain: false
this default doesn't really matter right?
This is fine with me I'm just making sure.
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.
From the web interface that's the behavior, by enabling enableAutoSubDomain=true
all branches listen. But when I tried from the CloudFormation without providing autoSubdomainCreationPatterns
it didn't seem to get a default so I tried to keep a similar behaviour
and yes, when enableAutoSubDomain=false
the autoSubdomainCreationPatterns
is filled but ignored
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license