-
Notifications
You must be signed in to change notification settings - Fork 61
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
fix(IAM Policy Management): add sort query parameter to List V2 Policies #238
Conversation
Signed-off-by: Shaun Colley <shaun.colley@ibm.com>
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.
@swcolley I found a few things to address:
- The generated service code has a bunch of references to the staging area because it looks like you used the "test" version of the API to generate the code. We need to use the production API (or the version that WILL be merged into production).
- At some point, we might want to consider adding pagination support to each of the list operations, especially for operations that might return a large number of items. Pagination would let the user deal with the "list" results one (smaller) page at a time.
- There's a new list_services operation, but there's no integration test or working examples code for it, so that will need to be added.
@@ -42,7 +42,7 @@ type IamPolicyManagementV1 struct { | |||
} | |||
|
|||
// DefaultServiceURL is the default URL to make service requests to. | |||
const DefaultServiceURL = "https://iam.cloud.ibm.com" | |||
const DefaultServiceURL = "https://iam.test.cloud.ibm.com" |
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.
Looks like you were using the "test" version of the API definition to generate code.
We really should be using the version that will be merged into the production branch so that we don't have references to the staging area in the generated code. This applies to the default service URL here as well as potentially other places where examples from the API definition are used to generate tests, etc.
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.
I used wrong branch so 1 and 3 should be addressed with latest commit.
For 2.) we have looked into pagination as part of SF review: Item 5 in this comment under Post-MVP.
@@ -290,7 +288,7 @@ func (iamPolicyManagement *IamPolicyManagementV1) ListPoliciesWithContext(ctx co | |||
// | |||
// Currently, only the `stringEquals` and the `stringMatch` operators are available. Resource attributes may support one | |||
// or both operators. For more information, see [Assigning access by using wildcard | |||
// policies](https://cloud.ibm.com/docs/account?topic=account-wildcard). | |||
// policies](https://test.cloud.ibm.com/docs/account?topic=account-wildcard). |
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.
Another example of where the staging area reference appears due to the use of the "test" version of the API definition. There are others below, but I'll avoid pointing out each one :)
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.
🤦
Signed-off-by: Shaun Colley <shaun.colley@ibm.com>
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.
LGTM
Signed-off-by: Shaun Colley <shaun.colley@ibm.com>
## [0.32.1](v0.32.0...v0.32.1) (2023-03-01) ### Bug Fixes * **IAM Policy Management:** add sort query parameter to List V2 Policies ([#238](#238)) ([be7c94e](be7c94e))
🎉 This PR is included in version 0.32.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
PR summary
Added Sort query parameter to List V2 Policies
PR Checklist
Please make sure that your PR fulfills the following requirements:
Current vs new behavior
Did not have sort functionality for listing v2 policies, now does.
Does this PR introduce a breaking change?
Other information