feat: First swag at a comprehensive and complete app DB component. #2977
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Added resourceoptions to some things. We may need more.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new component, OLAppDatabase, designed to simplify the setup of application databases within the OL infrastructure. The component aims to reduce boilerplate and provide a standard configuration for typical applications. The addition of pyright to the project dependencies is also a welcome change for improving code quality through static type checking. Overall, the changes look good, but there are a few areas that could be improved for clarity and maintainability.
Summary of Findings
- Security Group Description: The description for the security group is too specific. It should be more generic to reflect the broader purpose of the security group.
- Vault Mount Point: The vault mount point is hardcoded to
learn-ai. This should be configurable to allow for reuse across different applications.
Assessment
The pull request introduces a useful component for simplifying database setup. The code is generally well-structured and addresses the intended purpose. However, there are a few points that need to be addressed before merging, particularly regarding the security group description and the hardcoded vault mount point. Addressing these comments will improve the component's flexibility and maintainability. Users should have others review and approve this code before merging.
| self.app_db_security_group = ec2.SecurityGroup( | ||
| f"{ol_db_config.app_name}-db-security-group-{stack_info.env_suffix}", | ||
| name=f"{ol_db_config.app_name}-db-security-group-{stack_info.env_suffix}", | ||
| description="Access control for the learn-ai database.", |
There was a problem hiding this comment.
The description for the security group is very specific to learn-ai. Consider making it more generic to reflect the broader purpose of the security group, such as "Access control for application databases".
| description="Access control for the learn-ai database.", | |
| description="Access control for application databases.", |
|
|
||
| self.app_db_vault_backend_config = OLVaultPostgresDatabaseConfig( | ||
| db_name=self.ol_db_config.app_db_name, | ||
| mount_point=f"{self.app_db_config.engine}-learn-ai", |
There was a problem hiding this comment.
The vault mount point is hardcoded to learn-ai. This limits the reusability of the component. Consider making this configurable via the OLAppDatabaseConfig class.
| mount_point=f"{self.app_db_config.engine}-learn-ai", | |
| mount_point=f"{self.app_db_config.engine}-{ol_db_config.app_name}", |
What are the relevant tickets?
Relates to #2955
Description (What does it do?)
First swing at a one stop shopping application database component.
How can this be tested?
Integrate the component into an existing app that usees the current boilerplate. And smile if it works :)