-
Notifications
You must be signed in to change notification settings - Fork 108
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(table): add aws_rds_db_maintenance_action #2430
feat(table): add aws_rds_db_maintenance_action #2430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @fyqtian, I’ve added a few initial review comments. When you get a chance, could you please take a look?
Additionally, it would be great to include more example queries in the documentation.
Hello @ParthaI I appreciate the time you took to review my code and offer constructive feedback. Your expertise has not only enhanced my understanding of the project but also motivated me to strive for better practices in my future contributions. Thank you once again for your support. I look forward to implementing your suggestions and continuing to learn from you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @fyqtian,
I’ve provided another set of review comments. Please take a look when you have a moment.
Additionally, I would appreciate it if you could review the following:
- Consider updating the table name to
aws_rds_pending_maintenance_action
for better clarity based on the API name. - Fix indentation, casing, title, and description in the queries.
- Ensure the queries are available in both SQLite and PostgreSQL formats as we have for other tables.
- If possible, please add an example query demonstrating a table join.
Thank you! 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fyqtian,
I'm not sure if all the changes have been pushed yet. Could you please review the following points?
- Update the table and file name to
aws_rds_pending_maintenance_action
(we should always use the singular form). - In the documentation, I noticed that reserved PostgreSQL keywords (e.g.,
SELECT
,FROM
,WHERE
,CASE
) are in uppercase. Please convert them to lowercase to maintain consistency with other table documentation. - Update the query title "Show if the maintenance action is for a cluster" to "Check if a maintenance action is for a cluster" for better readability.
- Ensure that the query is provided in both SQLite and PostgreSQL formats.
- Please include an example query demonstrating how to join this table with
aws_rds_db_instance
andaws_rds_db_cluster
. This will help users understand how this table can be used in combination with other tables.
Let me know if you need any clarification. Thanks!
"Hi @ParthaI, I apologize for my carelessness. I often struggle to adhere to standards, and I recognize this as a personal shortcoming. Thank you once again for your time and effort in handling this tedious work; it greatly encourages me to engage with the community. |
As you mentioned "Please include an example query demonstrating how to join this table with aws_rds_db_instance and aws_rds_db_cluster. This will help users understand how this table can be used in combination with other tables." I noticed that the aws_rds_db_cluster table has pending_maintenance_actions field and I think it is not necessary to write a join sql. select pending_maintenance_actions from aws_rds_db_cluster where pending_maintenance_actions is not null; |
Yes, I have noticed it. The tables Based on our table development standards, we could consider adding a separate table if the API supports pagination to fetch the complete dataset. Once the new table is implemented and merged into the main branch, we will deprecate the respective columns in |
Hi @fyqtian, I wanted to bring to your attention on a comment regarding the table naming in this pull request. You can find the specific feedback here: #2430 (review). Could you please review and address this? Thank you! |
…tenance_action related name
…tenance_action related name
Hi @ParthaI, after updating the files' names and related references, please confirm. Thank you |
Integration test logs
Logs
Example query results
Results