add autoDomain and autoDomainWaitFor option to config #356
Conversation
|
@jconstance-amplify I've re-raised a PR with autoDomainWaitFor option and added an integration test. Let me know what you think :) |
|
@bryan-hunter Thanks for updating the PR. I'll try to fiddle around with this today. |
src/index.ts
Outdated
| this.serverless.cli.log(`Waiting ${waitFor} seconds before starting deployment | ||
| because first time creating domain`); | ||
|
|
||
| await sleep(waitFor); |
There was a problem hiding this comment.
One thing I don't like here is waiting for the entire period. What do you think about polling every few seconds until the time elapses? Or is there a throttling concern for this API that I'm forgetting about?
There was a problem hiding this comment.
agreed - this should only happen for the first deployment, but I did almost implement that before raising the PR, but wanted to get feedback first in case it could be avoided.
that being said, I'll add this now :) thanks!
There was a problem hiding this comment.
@jconstance-amplify I added a polling interval of 3 seconds (didn't seem worth making configurable). Let me know what you think :)
|
Kudos, SonarCloud Quality Gate passed!
|
| const atLeastOneDoesNotExist = () => this.domains.some((domain) => !domain.domainInfo); | ||
| const maxWaitFor = parseInt(this.serverless.service.custom.customDomain.autoDomainWaitFor, 10) || 120; | ||
| const pollInterval = 3; | ||
| for (let i = 0; i * pollInterval < maxWaitFor && atLeastOneDoesNotExist() === true; i++) { |
There was a problem hiding this comment.
How do we exit this for loop?
There was a problem hiding this comment.
if all domains exist , then atLeastOneDoesNotExist() will be false and exit (await this.getDomainInfo(); in the for loop will repopulate data so it can go from true/false)
otherwise, if it never populates, the i * pollInterval will eventually be greater than maxWaitFor and it'll exit and the deployment will eventually fail in the same way as if you never ran create_domain
There was a problem hiding this comment.
@jconstance-amplify let me know what you think :)
There was a problem hiding this comment.
@jconstance-amplify bump :)
There was a problem hiding this comment.
@jconstance-amplify no vacations allowed!!
There was a problem hiding this comment.
Fortunately for you I came back on Monday! :D
|
I ran into this issue today! This would be a great addition because although it's clearly highlighted here in the documentation I made the assumption that the |
There was a problem hiding this comment.
Apologies for the long wait @bryan-hunter. I was able to get some time to look at this yesterday evening, and seems to work okay for me.
I'll go ahead and merge this now. Thanks!
|
Thank you so much! |
addressed feedback on #323
Changes proposed in this pull request:
autoDomainandautoDomainWaitForoption 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.Due to eventual consistency,
autoDomainWaitForoption is defaulted to 120 seconds but it should only ever have to wait on the first time it is created.