-
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 Policy Management): re-gen service after recent API changes,… #259
feat(IAM Policy Management): re-gen service after recent API changes,… #259
Conversation
… Tests added for Policy Template Signed-off-by: Vivek.Jain3 <[email protected]>
Signed-off-by: Vivek.Jain3 <[email protected]>
Signed-off-by: Vivek.Jain3 <[email protected]>
…dded for PolicyAssignment by id Signed-off-by: Vivek.Jain3 <[email protected]>
…ignment by id Signed-off-by: Vivek.Jain3 <[email protected]>
testPolicyTemplateETag string = "" | ||
testPolicyTemplateVersion string = "" | ||
// change testPolicyAssignmentId id after prod account setup | ||
testPolicyAssignmentId = "442c6fc4-2f74-41b2-bf4d-98342614cd22" |
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.
Why not use a flow like:
- list policy assignments 2. Store an assignment id, 3. use stored id to call get assignment by id?
I think it is okay to hardcode an ID here, but I'm not sure if it matters whether it is production or test environment. These tests are for us to run locally and we can give production credentials in theiam_policy_management.env
when we test locally to verify works in production.
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 believe it is better to use assignment id from list policy assignments
, as same assignment id
will not be available in different accounts so even if we hardcode it will restricted to use same account.
Thanks i'll update
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 updated the tests
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 think your reasoning sounds good. We can try to make sure we create an assignment in the account we use for testing, but luckily there are some we can refer to currently.
Thanks for making that change.
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 might not understand 100% of this discussion, but it sounds like you need to use an id of a pre-existing account that your tests rely on. The int tests and examples for this service already retrieve the "TEST_ACCOUNT_ID" config property from the .env file during startup. It is stored in the testAccountID
variable in the integration tests and in exampleAccountID
in the examples code.
It's way better to retrieve this value from the .env file than to keep having to update the hard-coded id within the actual source code.
examplePolicyTemplateETag string | ||
examplePolicyTemplateVersion string | ||
// change testPolicyAssignmentId id after prod account setup | ||
testPolicyAssignmentId = "442c6fc4-2f74-41b2-bf4d-98342614cd22" |
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.
Same as other comment.
Signed-off-by: Vivek.Jain3 <[email protected]>
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 good from my perspective :D
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.
There are some improvements that can be made in the integration tests and examples. I'd like to encourage you to update the API definition to define links where there are inter-operation dependencies. This will then let you avoid the manual changes to store off specific values and then use those values in downstream operations.
The tests and examples code are functional as they are, but the change I'm suggesting will make for easier maintenance in the future.
Let's discuss before we merge...
Expect(response.StatusCode).To(Equal(201)) | ||
Expect(policyTemplate).ToNot(BeNil()) | ||
|
||
examplePolicyTemplateID = *policyTemplate.ID |
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.
If you define the appropriate link objects in your API definition, the SDK generator will generate code like this to store off a value such as the policyTemplate.ID value, then generate code in the test of the consuming operation to use the stored value to initialize an operation parameter.
Take a look at the SDK squad's mock service that we use for various types of testing:
https://github.ibm.com/CloudEngineering/openapi-sdkgen/blob/main/src/test/resources/ansiform-service.yaml
and the associated Go code in the resulting Go SDK:
https://github.ibm.com/CloudEngineering/ansiform-go-sdk/tree/main/ansiformmockv1
Note that the integration tests and examples code were generated for this service with no required manual post-gen changes. This is due to the fact that example values and links are defined throughout the API definition.
Expect(policyTemplate.Name).ToNot(BeNil()) | ||
Expect(policyTemplate.Policy).ToNot(BeNil()) | ||
|
||
examplePolicyTemplateETag = response.GetHeaders().Get("ETag") |
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.
You can also use a link to store off ETag values and then use the value to initialize an "IfMatch" header param in downstream operations.
Expect(policyTemplate.Policy.Type).To(Equal(core.StringPtr("access"))) | ||
Expect(policyTemplate.AccountID).To(Equal(core.StringPtr(testAccountID))) | ||
|
||
testPolicyTemplateID = *policyTemplate.ID |
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.
Same comments here re: links/examples as in the examples code above.
}) | ||
It(`ListPolicyTemplates(listPolicyTemplatesOptions *ListPolicyTemplatesOptions)`, func() { | ||
listPolicyTemplatesOptions := &iampolicymanagementv1.ListPolicyTemplatesOptions{ | ||
AccountID: core.StringPtr(testAccountID), |
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.
AccountID: core.StringPtr(testAccountID), | |
AccountID: &testAccountID, |
The core.StringPtr() function is only needed when using a string literal. For a variable, we can just use the & operator directly.
Signed-off-by: Vivek.Jain3 <[email protected]>
Signed-off-by: Vivek.Jain3 <[email protected]>
… Tests update for Policy Template Signed-off-by: Vivek.Jain3 <[email protected]>
# [0.43.0](v0.42.1...v0.43.0) (2023-08-09) ### Features * **IAM Policy Management:** re-gen service after recent API changes,… ([#259](#259)) ([2837a98](2837a98))
🎉 This PR is included in version 0.43.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
PR summary
Added feature support for Enterprise IAM work, policy templates and get template assignments.
Issue: https://github.ibm.com/IAM/AM-issues/issues/1086
PR Checklist
Please make sure that your PR fulfills the following requirements:
Current vs new behavior
Does this PR introduce a breaking change?
Other information
API defintion:
Staging
Prod
Test information:
Unit Tests:
Example Tests:
Integration Tests: