Skip to content

Conversation

@radito3
Copy link
Contributor

@radito3 radito3 commented Dec 8, 2020

Fixes #1078

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 8, 2020

CLA Signed

The committers are authorized under a signed CLA.

@twoseat
Copy link
Contributor

twoseat commented Dec 9, 2020

Thanks for the PR @radito3! I did a quick check through the code and the only thing that jumped out at me is we're naming all new V3 endpoints with their version, e.g. RolesV3. If you could update that, and sign the EasyCLA, I should be able to get this merged quickly.

@radito3
Copy link
Contributor Author

radito3 commented Dec 10, 2020

@twoseat Thanks for the comment! I renamed the appropriate classes from Roles to RolesV3 and signed the CLA. Could you take another look at the pr?

Copy link
Contributor

@twoseat twoseat left a comment

Choose a reason for hiding this comment

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

Looks great with just a couple of issues. Once updated I'll put some integration tests behind it, which almost always reveal a problem (generally a gap between the API and the documentation, not us!)

import org.cloudfoundry.client.v3.ToOneRelationship;
import org.junit.Test;

public class CreateRoleRequestTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a couple of tests for the @Value.Check in the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

import static org.cloudfoundry.operations.applications.ApplicationHealthCheck.PORT;
import static org.junit.Assume.assumeTrue;

@org.junit.Ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my bad. Will remove it

@radito3
Copy link
Contributor Author

radito3 commented Dec 18, 2020

@twoseat Could you take another look?

@twoseat twoseat self-assigned this Jan 13, 2021
@twoseat twoseat added this to the 4.14.0.RELEASE milestone Jan 13, 2021
@twoseat twoseat closed this in 52a0557 Jan 14, 2021
@radito3 radito3 deleted the v3-roles branch January 6, 2022 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing V3 Roles API

2 participants