add autoDomain option to config #323
Conversation
|
@jconstance-amplify any chance you will be able to review this? Thanks!! |
|
@jconstance-amplify bump - looking for feedback or approval when you have some free time 😁 thanks again! |
There was a problem hiding this comment.
Thanks for sending this PR. Apologies for the long wait on review. One minor comment on the name of the config option. Otherwise seems okay.
We'll probably wait to merge this until after #319 though. And you might need to fix some merge conflicts after that.
README.md
Outdated
| @@ -85,6 +86,7 @@ custom: | |||
| | hostedZonePrivate | | If hostedZonePrivate is set to `true` then only private hosted zones will be used for route 53 records. If it is set to `false` then only public hosted zones will be used for route53 records. Setting this parameter is specially useful if you have multiple hosted zones with the same domain name (e.g. a public and a private one) | | |||
| | enabled | true | Sometimes there are stages for which is not desired to have custom domain names. This flag allows the developer to disable the plugin for such cases. Accepts either `boolean` or `string` values and defaults to `true` for backwards compatibility. | | |||
| securityPolicy | tls_1_2 | The security policy to apply to the custom domain name. Accepts `tls_1_0` or `tls_1_2`| | |||
| | createDomainName | `false` | Toggles whether or not the plugin will run `create_domain/delete_domain` as part of `sls deploy/remove` so that multiple commands are not required. | | |||
There was a problem hiding this comment.
I'm not sure about the name createDomainName because it doesn't quite convey that this will also automatically run delete_domain for you. Maybe something like autoDomain, automaticallyManageDomain, or something along those lines?
There was a problem hiding this comment.
I updated to autoDomain.
I'll keep an eye out for when 319 is merged - thanks!
bryan-hunter
changed the title
…in-manager into add-create-domain-to-deploy
…in-manager into add-create-domain-to-deploy
|
@jconstance-amplify I have pulled in master and addressed merge conflicts when you have a chance to review |
|
@jconstance-amplify bumping, just in case you missed it :) |
|
@bryan-hunter Thanks for the bump, will try to get this in this week. |
There was a problem hiding this comment.
LGTM, apologies for the delay and thanks for doing this @bryan-hunter.
|
Kudos, SonarCloud Quality Gate passed!
|
|
@jconstance-amplify do you know when a new version will be deployed? |
|
@bryan-hunter Probably today or tomorrow. |
|
@bryan-hunter Hey, had to revert this off of master. I ran into a few intermittent race conditions with this locally. Looks like sometimes when the deploy tries to add the base path mapping, the domain isn't available yet and the deploy fails. Maybe you could look into that via adding an integration test for this new behavior? |
|
@jconstance-amplify from the docs: Do you think I should add an optional |
|
@bryan-hunter I believe the initialized time is how long it takes for it to be available to handle web traffic - the timing for it to be available inside of the API is usually much less. This is probably an issue stemming how from the AWS API is usually eventually consistent. If you create a resource, it's not unexpected for subsequent API calls to not see the resource for some amount of time (in my experience usually less than a few minutes). Adding in the config option and defaulting it to something like a few minutes seems like it would resolve the issue to me. |
|
It has been implemented in latest version? |
there's an open PR for it here: #356 |
Changes proposed in this pull request:
autoDomainoption to customDomain configThis will enable the creation/deletion of a domain name to be run as part of
sls deployandsls remove.This will be an option to eliminate the need to call
sls create_domainprior tosls deployif you'd like to only runsls deployacross all of your stacks and do not want to run an additional pre-command.