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

Prerender Fargate: Recaching implementation #1107

Merged
merged 4 commits into from
Oct 15, 2023

Conversation

krishanthisera
Copy link
Contributor

Prerender Fargate: Recaching implementation


Description of the proposed changes

  • Include prerender recache functionality with URL Token association
  • Fine-tune the documentation
  • Include eslint rule for @typescript-eslint/no-unused-vars
  • Separate the props from the actual construct

Screenshots (if applicable)

  • N/A

Other solutions considered (if any)

  • N/A

Notes to PR author

⚠️ Please make sure the changes adhere to the guidelines mentioned here

Notes to reviewers

  • The re-caching logic has not been altered in this PR

🛈 When you've finished leaving feedback, please add a final comment to the PR tagging the author, letting them know that you have finished leaving feedback

@crispy101
Copy link
Contributor

The changes look good to me in general, but can you add the below as we discussed before:

  • README update around Recaching: how it works, how to enable and configure, etc.
  • API G/W endpoint URL in the CFN output

@TheOrangePuff
Copy link
Contributor

TheOrangePuff commented Oct 13, 2023

@krishanthisera just a comment, can we please keep formatting changes in their own PR? It makes it really hard to tell what has functionally changed

Ah, wait, realised you moved props to a new file, not a reformat. Ignore me 😅

Copy link
Contributor

@TheOrangePuff TheOrangePuff left a comment

Choose a reason for hiding this comment

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

LGTM! Just some minor comments but happy for this to go through 🙂


/**
* Options for configuring the Prerender Fargate construct.
* `PrerenderFargate` construct sets up an AWS Fargate service to run a
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really cool 🙏 We should do this for all our constructs

bucketName,
domainName,
prerenderName,
} = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like using props.{blah} makes it clearer to see where the variables are coming from. But maybe that's just me 🤷‍♂️ If no one else has an issue with this I don't mind 🙂

@krishanthisera
Copy link
Contributor Author

krishanthisera commented Oct 15, 2023

The changes look good to me in general, but can you add the below as we discussed before:

* README update around Recaching: how it works, how to enable and configure, etc.

* API G/W endpoint URL in the CFN output

The changes look good to me in general, but can you add the below as we discussed before:

* README update around Recaching: how it works, how to enable and configure, etc.

* API G/W endpoint URL in the CFN output

@crispy101 We shall create two new tickets for the task and add to the epic 🤞

@krishanthisera krishanthisera merged commit a66170e into epic/cdk-v2 Oct 15, 2023
1 check 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.

3 participants