-
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
feat(IAM Identity): add support for inactivity reports #181
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.
Just had a couple comments to resolve before we merge...
iamidentityv1/iam_identity_v1.go
Outdated
type IamIdentityV1 struct { | ||
Service *core.BaseService | ||
} | ||
|
||
// 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.
This indicates that you generated the code using the "test" version of the API definition. We should really be using the version that will be checked into the "production" branch. That version would (I assume) specify the prod endpoint URL.
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.
Right, sorry this was by mistake
Reference: &reportId, | ||
} | ||
|
||
for { |
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.
It looks like the logic here (with the "infinite" for loop) is to invoke the operation until it returns a status code other than 204. How do we know when/if we'll ever get a non-204 status code back??? This almost seems like a "paginated" response but there's no 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.
We are generating a report and while report generation is in progress it returns a 204. At some point in time the report will be either complete (and thus return a 200) or failed and return an error. But I accept that we should have a defensive strategy and stop looping after a while. Will improve...
@michaelbeck Could you please post a screenshot of the results when running the integration tests and examples? |
You see the screenshot of the integration tests under "Other Information" Will add one for the examples... Here comes the examples (just copying the end of the test, as these are the only added test cases): |
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
I'll merge this in when the rest of the PRs are ready as well.
d0d83d6
to
d2acb41
Compare
# [0.24.0](v0.23.0...v0.24.0) (2022-04-06) ### Features * **IAM Identity:** add support for inactivity reports ([#181](#181)) ([b332e0e](b332e0e))
🎉 This PR is included in version 0.24.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
PR summary
adding new endpoints: create report, get report
adding new optional query parameter include_activity for the existing get calls of apikey/serviceid/profile by id.
PR Checklist
Please make sure that your PR fulfills the following requirements:
Current vs new behavior
users will now be able to generate report for inactive identities.
create report: will return a reference
get report: this will return a detailed report of inactive identities
existing get apikey/serviceid/profile by id will now include additional optional parameter include_activity
if include_activity is passed as true the response will include a new section activity with last authn time and count.
Does this PR introduce a breaking change?
Other information