Skip to content
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

Support for Multiple domains #327

Merged
merged 13 commits into from Sep 23, 2020

Conversation

ConradKurth
Copy link
Contributor

@ConradKurth ConradKurth commented Apr 28, 2020

Fixes #88

Description of Issue Fixed
Wanted support to be able to specify multiple domains to be created

Changes proposed in this pull request:

Instead of storing the domain inside the ServerlessCustomDomain class, I moved those variables to a new class that would handle the initialization of all those variables.

In the class ServerlessCustomDomain, I made an array of the domains we wish to make an iterate over them to create the multiple domains

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tehnrd
Copy link
Contributor

tehnrd commented Apr 30, 2020

A heads up that there is a lot of potential overlap/conflict with #319 . #319 also follows a very similar approach of breaking the domains/HTTP type apart and running them through the process: https://github.com/amplify-education/serverless-domain-manager/pull/319/files#diff-ed009b6b86b017532ef0489c77de5100R148

I think #319 PR could be fixed with a small tweak here to support multiple domains, https://github.com/amplify-education/serverless-domain-manager/pull/319/files#diff-ed009b6b86b017532ef0489c77de5100R243 , but I haven't investigated it thoroughly yet.

@ConradKurth
Copy link
Contributor Author

@tehnrd Hmm, just skimming things briefly it might be easy to do since you broke the stuff into an array. Basically a lot of the work was just having things be able to run in a loop, the only secret sauce after that is just being able to have customDomain vs customDomains notice the s :). So yeah, thats fine by me if they want to merge yours in and I can make a new PR against that

@tehnrd
Copy link
Contributor

tehnrd commented Apr 30, 2020

Cool cool, I hate seeing all the work in this PR be blocked by mine :-/ but I think that will be the least amount of conflicts in the end.

I'll give yours a deeper dive tomorrow and see what good parts we can pull out.

index.ts Outdated Show resolved Hide resolved
@ConradKurth
Copy link
Contributor Author

@tehnrd All good about the work! I would rather just see the change be realized haha, i have no attachments to it :). But i will leave some comments on important places

domain.ts Outdated Show resolved Hide resolved
@florianbepunkt
Copy link

Thank you for the PR. Is there an ETA when this will land? For me it is unclear how this works: I want two domains (mydomain.com, mydomain.co.uk) to point to the same websocket api (same stage). How would you specify this in serverless.yml? Maybe an example / explanation in the param description would be helpful.

@ConradKurth
Copy link
Contributor Author

@florianbepunkt so i have not looked at this before all those changes were made, i need to look into this again, some larger changes were done unfortunately

@ConradKurth
Copy link
Contributor Author

@eunanhardy this is ready to be merged i believe

@sonarcloud
Copy link

sonarcloud bot commented Aug 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 1 Security Hotspot to review)
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@eunanhardy
Copy link

@tehnrd All Tests Passing, No merge conflicts on my end, seems to work like a charm. good work @ConradKurth

@ConradKurth
Copy link
Contributor Author

@eunanhardy is there are timeline on getting these changes in?

@rddimon rddimon merged commit 3134432 into amplify-education:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple domain support
5 participants