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

feat!: dbtp 928 add cdn endpoint module #141

Merged
merged 20 commits into from
Jun 4, 2024
Merged

feat!: dbtp 928 add cdn endpoint module #141

merged 20 commits into from
Jun 4, 2024

Conversation

ejayesh
Copy link
Contributor

@ejayesh ejayesh commented May 30, 2024

This change introduces a new module called CDN, this will enable the deployment of a CloudFront endpoint.
There are breaking changes in extensions.yml to facilitate this change.

Upgrade path: in extensions.yml update type: alb "cdn_domains_list" variable to include applications internal prefix. See README.md/CDN for example.

@ejayesh ejayesh changed the title feature: dbtp 928 add cdn endpoint module feat: dbtp 928 add cdn endpoint module May 30, 2024
@ejayesh ejayesh requested a review from a team May 30, 2024 08:09
.checkov.baseline Outdated Show resolved Hide resolved
@WillGibson
Copy link
Contributor

Title should start feat!: to indicate breaking change.

Pull request description should include Upgrade Path, for easy copying into the release notes.

@ejayesh ejayesh changed the title feat: dbtp 928 add cdn endpoint module feat!: dbtp 928 add cdn endpoint module May 30, 2024
Copy link
Contributor

@WillGibson WillGibson left a comment

Choose a reason for hiding this comment

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

Partial pass. Mostly comments on the documentation.

There's a lot to grok in the actual Terraform, but the tests feel kind of light by comparison.

.checkov.baseline Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cdn/locals.tf Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@lgarvey lgarvey left a comment

Choose a reason for hiding this comment

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

The terraform looks fine. Do we need some documentation?

@ejayesh
Copy link
Contributor Author

ejayesh commented May 31, 2024

The terraform looks fine. Do we need some documentation?

I updated the readme and updated the platform docs

cdn/main.tf Outdated Show resolved Hide resolved
Copy link
Contributor

@WillGibson WillGibson left a comment

Choose a reason for hiding this comment

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

For the bits I can meaningfully grok, mostly the documentation stuff, I'm adding my approval to @lgarvey's for the Terraform parts.

@ejayesh ejayesh merged commit 20d6f5b into main Jun 4, 2024
6 checks passed
@ejayesh ejayesh deleted the DBTP-928-add-cdn branch June 4, 2024 07:15
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.

4 participants