Skip to content

Add Domains V3 resource to CloudFoundryClient #1030

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add Domains V3 resource to CloudFoundryClient #1030

wants to merge 2 commits into from

Conversation

@LittleBaiBai
Copy link
Contributor

[#1008]

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 24, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@LittleBaiBai LittleBaiBai requested a review from nebhale January 24, 2020 19:39
@twoseat twoseat self-assigned this Jan 27, 2020
@LittleBaiBai
Copy link
Contributor Author

Hi @twoseat, do you have any feedbacks or concern on this PR?

twoseat
twoseat suggested changes Mar 10, 2020
View reviewed changes
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.

Thanks for the PR! Everything looks really good apart from the breaking change noted and a few minor formatting things (I think I caught all the instances of these, hence there are a lot of comments but nothing major to change).

It would be enormously helpful to me if you could add in Integration Tests. I understand that you almost certainly won't be able to run them, which means there will almost certainly be problems with them. But just having the structure there makes it easier to get through the merge, so I won't judge you on any mistakes!

...client-reactor/src/main/java/org/cloudfoundry/reactor/client/_ReactorCloudFoundryClient.java Outdated
import org.cloudfoundry.client.v2.blobstores.Blobstores;
import org.cloudfoundry.client.v2.buildpacks.Buildpacks;
import org.cloudfoundry.client.v2.domains.Domains;
import org.cloudfoundry.client.v2.domains.DomainsV2;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change - ideally we would have thought ahead and called it DomainsV2 from the beginning, but we are where we are, so this needs to remain unchanged. Introducing DomainsV3 and ReactorDomainsV3 is fine, even if it looks a little inconsistent.

...client-reactor/src/main/java/org/cloudfoundry/reactor/client/_ReactorCloudFoundryClient.java Outdated
import org.cloudfoundry.reactor.client.v2.blobstores.ReactorBlobstores;
import org.cloudfoundry.reactor.client.v2.buildpacks.ReactorBuildpacks;
import org.cloudfoundry.reactor.client.v2.domains.ReactorDomains;
import org.cloudfoundry.reactor.client.v2.domains.ReactorDomainsV2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change (see above)

...client-reactor/src/main/java/org/cloudfoundry/reactor/client/_ReactorCloudFoundryClient.java
Comment on lines 172 to 181
public Domains domains() {
return new ReactorDomains(getConnectionContext(), getRootV2(), getTokenProvider(), getRequestTags());
public DomainsV2 domainsV2() {
return new ReactorDomainsV2(getConnectionContext(), getRootV2(), getTokenProvider(), getRequestTags());
}

@Override
@Value.Derived
public DomainsV3 domainsV3() {
return new ReactorDomainsV3(getConnectionContext(), getRootV3(), getTokenProvider(), getRequestTags());
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change (see above)

...lient-reactor/src/main/java/org/cloudfoundry/reactor/client/v2/domains/ReactorDomainsV2.java Outdated
import org.cloudfoundry.client.v2.domains.DeleteDomainRequest;
import org.cloudfoundry.client.v2.domains.DeleteDomainResponse;
import org.cloudfoundry.client.v2.domains.Domains;
import org.cloudfoundry.client.v2.domains.DomainsV2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change (see above)

...lient-reactor/src/main/java/org/cloudfoundry/reactor/client/v2/domains/ReactorDomainsV2.java Outdated

/**
* The Reactor-based implementation of {@link Domains}
* The Reactor-based implementation of {@link DomainsV2}
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change (see above)

cloudfoundry-client/src/main/java/org/cloudfoundry/client/v3/organizations/OrganizationsV3.java Outdated
* @param request the List Organization Domains request
* @return the response from the List Organization Domains request
*/
Mono<ListOrganizationDomainsResponse> listDomains(ListOrganizationDomainsRequest request); //TODO needs tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it still need tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I added test but forgot to remove the TODO 😅

.../src/main/java/org/cloudfoundry/client/v3/organizations/_ListOrganizationDomainsRequest.java Outdated
Comment on lines 33 to 65
/**
* The organization id
*/
@JsonIgnore
abstract String getOrganizationId();

/**
* The domain ids to filter by
*/
@FilterParameter("guids")
@Nullable
abstract List<String> getDomainIds();

/**
* The names
*/
@FilterParameter("names")
@Nullable
abstract List<String> getNames();

/**
* The owning organization ids
*/
@FilterParameter("organization_guids")
@Nullable
abstract List<String> getOwningOrganizationIds();

/**
* The metadata query
*/
@FilterParameter("label_selector")
@Nullable
abstract String getLabelSelector();
Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetical order

cloudfoundry-operations/src/test/java/org/cloudfoundry/operations/AbstractOperationsTest.java Outdated
import org.cloudfoundry.client.v2.applications.ApplicationsV2;
import org.cloudfoundry.client.v2.buildpacks.Buildpacks;
import org.cloudfoundry.client.v2.domains.Domains;
import org.cloudfoundry.client.v2.domains.DomainsV2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change (see above)

cloudfoundry-operations/src/test/java/org/cloudfoundry/operations/AbstractOperationsTest.java Outdated
protected final CloudFoundryClient cloudFoundryClient = mock(CloudFoundryClient.class, RETURNS_SMART_NULLS);

protected final Domains domains = mock(Domains.class, RETURNS_SMART_NULLS);
protected final DomainsV2 domainsV2 = mock(DomainsV2.class, RETURNS_SMART_NULLS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change (see above)

cloudfoundry-operations/src/test/java/org/cloudfoundry/operations/AbstractOperationsTest.java Outdated
when(this.cloudFoundryClient.applicationsV3()).thenReturn(this.applicationsV3);
when(this.cloudFoundryClient.buildpacks()).thenReturn(this.buildpacks);
when(this.cloudFoundryClient.domains()).thenReturn(this.domains);
when(this.cloudFoundryClient.domainsV2()).thenReturn(this.domainsV2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change (see above)

@LittleBaiBai LittleBaiBai force-pushed the domains-v3 branch 2 times, most recently from 955d12f to 9403974 Compare April 17, 2020 16:14
@LittleBaiBai LittleBaiBai requested a review from twoseat April 21, 2020 15:24
@LittleBaiBai
Copy link
Contributor Author

@twoseat Thanks for your thorough review! I believe I have addressed all the comments you left and made sure no DomainsV2 showed up in a global search.

I also added integration tests, but I may need some help with getting one of them to pass - DomainsTest.delete. This test fails because delete is an asynchronous operation and the API spec declared the response to have job location url, but the CF instance I'm using is not returning it. So I wasn't able to wait until job is done as in some other integration test, then the assertion right after deletion fails. We could add retries or arbitrary wait time, but I'd love to hear your thoughts on this first.

@twoseat
Copy link
Contributor

twoseat commented May 6, 2020

Thanks for the updates @LittleBaiBai - I'm working on it now, and have found what I guess is your domains issue (there's an undocumented warnings field returned as part of the job, and we're configured to choke on unexpected fields for test purposes). I've added something temporarily that makes the test work, but this issue (and possibly other V3 stuff) might have to wait until I get some feedback on what the field is supposed to contain.

@twoseat twoseat closed this in edff248 May 13, 2020
@twoseat twoseat added this to the 3.24.0.RELEASE milestone May 21, 2020
@LittleBaiBai LittleBaiBai deleted the domains-v3 branch July 15, 2020 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nebhale nebhale Awaiting requested review from nebhale

@twoseat twoseat Awaiting requested review from twoseat

Assignees

@twoseat twoseat

Labels

None yet

Projects

None yet

Milestone

3.24.0.RELEASE

Development

Successfully merging this pull request may close these issues.

2 participants

@LittleBaiBai @twoseat