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

Improve Roles example for OpenAPI and low level OpenAPI #333

Merged
merged 9 commits into from
Oct 2, 2020

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Sep 2, 2020

This PR builds on top of #325 to further improve an example with Roles as per initial design improvements:

  • Type name. OpenApiRole becomes just Role as in the end maybe user shouldn’t care what type of API is used – he just needs result in high level API.
  • GetAllOpenApiRoles function now returns []*Role (with client wrapped in) instead of []*types.Role. That way one can immediatelly call Update and Delete on result.
  • CreateRole function has a pointer receiver of parent (*adminOrg in this case).
  • OpenApiGetAllItems will default pageSize to 128 results (maximum) per page unless caller supplies a value.

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Thanks!

govcd/openapi.go Outdated
@@ -75,9 +77,15 @@ func (client *Client) OpenApiGetAllItems(apiVersion string, urlRef *url.URL, que
return fmt.Errorf("OpenAPI is not supported on this VCD version")
}

// Page size is defaulted to 128 (maximum supported number) to reduce HTTP calls and improve performance unless caller
// provides other value
const pageSize = "128"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do really need this constant? defaultPageSize(queryParams, "128")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My point was to emphasize it. Hopefully a comment above is sufficient. Removed.

@Didainius Didainius merged commit 332c4fa into vmware:master Oct 2, 2020
@Didainius Didainius deleted the openapi-adjust branch October 2, 2020 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants