-
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
Add table aws_trusted_advisor_check_summary Closes #1831 #1978
Conversation
I wasn't able to personally test the pull request because we lack a business, Enterprise On-Ramp, or Enterprise Support plan, which is required for making the |
This PR is good to review now. It has been tested by the user(#1831 (comment)). |
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.
@ParthaI please have a look at the comments, thanks!
Co-authored-by: Madhushree Ray <[email protected]>
Co-authored-by: Madhushree Ray <[email protected]>
Co-authored-by: Madhushree Ray <[email protected]>
Co-authored-by: Madhushree Ray <[email protected]>
Co-authored-by: Madhushree Ray <[email protected]>
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
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.
@ParthaI please take a look at the review comments. Thanks!!
} | ||
|
||
input := &support.DescribeTrustedAdvisorChecksInput{ | ||
Language: aws.String("en"), |
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.
@ParthaI can this be considered as an optional qual?
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.
@misraved, According to the API documentation, this parameter is necessary for making the API call. We can designate it as a required qualifier or as an optional qualifier with the default value set to en
(if the user doesn't provide any value in the WHERE clause for it). By the way, the API does not return the 'Language' property in its response.
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.
@ParthaI I think we can update the table to use language
as a required qual to best replicate the API behaviour.
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.
Updated, the PR by making the language column as required qual.
} | ||
|
||
// List call | ||
result, err := svc.DescribeTrustedAdvisorChecks(ctx, input) |
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.
Does the API support pagination?
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.
No, the API does not support pagination
@ParthaI could you please update the example queries to use the required |
Integration test logs
Logs
Example query results
Results