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

Gracefully handle redirectToMainDomain with only a single domain #321

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

aran112000
Copy link
Contributor

We've run into issues with dynamic configs which are stage-dependent - different stages have different domain(s).

This change allows the directive to be present but will silence its behaviour where only a single domain has been specified.

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great thank you!

@mnapoli
Copy link
Member

mnapoli commented Apr 26, 2023

The tests are failing, any idea why?

@aran112000
Copy link
Contributor Author

The tests are failing, any idea why?

I believe these should all be good to go now :)

@@ -372,7 +372,7 @@ export class ServerSideWebsite extends AwsConstruct {
private createRequestFunction(): cloudfront.Function {
let additionalCode = "";

if (this.configuration.redirectToMainDomain === true) {
if (this.configuration.redirectToMainDomain === true && this?.domains && this.domains.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build still fails, could it be that this? is invalid because this cannot be null?

@aran112000 aran112000 closed this Jun 9, 2023
@aran112000 aran112000 force-pushed the server-side-website-improvements branch from 972ed7c to 71b1580 Compare June 9, 2023 16:52
Far better way to handle the same solution. Plus it fixes the CI
@aran112000 aran112000 reopened this Jun 9, 2023
@mnapoli
Copy link
Member

mnapoli commented Jun 16, 2023

Thanks!

@mnapoli mnapoli merged commit c075681 into getlift:master Jun 16, 2023
28 checks passed
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.

None yet

2 participants