Skip to content
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(enterprise manager): update enterprise management after recent api changes #107

Merged
merged 18 commits into from
Apr 7, 2021

Conversation

Andras-Csanyi
Copy link
Contributor

@Andras-Csanyi Andras-Csanyi commented Apr 7, 2021

PR summary

This PR updates Enterprise Manager Service SDK.

Old integration tests using Account Manager private api endpoints has been removed. There are SDK methods still untested due to that they require account creation programatically, and unfortunateley we don't have that yet.

These methods:

  • CreateEnterprise
  • ImportAccountToEnterprise

PR Checklist

Please make sure that your PR fulfills the following requirements:

  • The commit message follows the Angular Commit Message Guidelines.
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Current vs new behavior

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@Andras-Csanyi Andras-Csanyi self-assigned this Apr 7, 2021
@Andras-Csanyi
Copy link
Contributor Author

@padamstx

Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few non-critical changes are needed, but I'm approving now and we can include the integration test changes in a follow-on PR.

BeforeEach(func() {
shouldSkipTest()
})
It(`CreateAccountGroup(createAccountGroupOptions *CreateAccountGroupOptions)`, func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a cleaner way to create 2 account groups would be to have two separate It() blocks, with each one creating a single account group.

BeforeEach(func() {
shouldSkipTest()
})
It(`ListAccountGroups(listAccountGroupsOptions *ListAccountGroupsOptions)`, func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's re-code this test so that it uses a loop to retrieve a page at a time (perhaps use limit=1?) and accumulate the results in a slice, similar to how the user-management int tests exercise the list-type operations.


It("Successfully Create Account", func() {
shouldSkipTest()
listEnterprisesOptions := &enterprisemanagementv1.ListEnterprisesOptions{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to exercise pagination for this operation as well.

@padamstx padamstx assigned padamstx and unassigned Andras-Csanyi Apr 7, 2021
@padamstx padamstx merged commit 3a11a67 into main Apr 7, 2021
@padamstx padamstx deleted the update-enterprise-management branch April 7, 2021 19:58
ibm-devx-automation pushed a commit that referenced this pull request Apr 7, 2021
## [0.18.6](v0.18.5...v0.18.6) (2021-04-07)

### Bug Fixes

* **enterprise manager:** update enterprise management after recent api changes ([#107](#107)) ([3a11a67](3a11a67))
@ibm-devx-automation
Copy link

🎉 This PR is included in version 0.18.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants