Add routes support for V3 #1012
Conversation
There was a problem hiding this comment.
Thanks for the extensive PR @enchobelezirev! I've made some comments but nothing major.
| import org.cloudfoundry.client.v3.applications.UpdateApplicationRequest; | ||
| import org.cloudfoundry.client.v3.applications.UpdateApplicationResponse; | ||
| import org.cloudfoundry.client.v3.applications.ListApplicationRoutesRequest; | ||
| import org.cloudfoundry.client.v3.applications.*; |
There was a problem hiding this comment.
House style is to specify all imports, no wildcards.
| import org.cloudfoundry.client.v3.spaces.ListSpacesRequest; | ||
| import org.cloudfoundry.client.v3.spaces.ListSpacesResponse; | ||
| import org.cloudfoundry.client.v3.spaces.SpacesV3; | ||
| import org.cloudfoundry.client.v3.spaces.*; |
There was a problem hiding this comment.
House style is to specify all imports, no wildcards.
| * The metadata query | ||
| */ | ||
| @FilterParameter("label_selector") | ||
| @Nullable |
There was a problem hiding this comment.
FilterParameter has an implicit Nullable so this shouldn't be necessary. Having said that I've noticed I've done it in one or two places, so either we've both made the same mistake or there's a reason for this that I've forgotten. Can you clarify?
There was a problem hiding this comment.
I have looked how to make the POJOs from the already existing places :) So, if it is mistake, I could remove the @nullable from everywhere
There was a problem hiding this comment.
I think remove it for now, if it causes problems it should turn up in integration testing and I'll fix it then. I did just notice that the javadoc you have is wrong for this though.
| @FilterParameter("domain_guids") | ||
| abstract List<String> getDomainGuids(); | ||
|
|
||
|
|
| * The metadata | ||
| */ | ||
| @JsonProperty("metadata") | ||
| @Nullable |
There was a problem hiding this comment.
See earlier Nullable comment
There was a problem hiding this comment.
Is this applicable for the @JsonProperty annotation?
There was a problem hiding this comment.
You're right, I'm wrong, ignore this one!
| * The metadata query | ||
| */ | ||
| @FilterParameter("label_selector") | ||
| @Nullable |
There was a problem hiding this comment.
see earlier Nullable comment
| */ | ||
| @Nullable | ||
| @JsonProperty("host") | ||
| public abstract String getHost(); |
There was a problem hiding this comment.
Public shouldn't be needed here
| * The route id | ||
| */ | ||
| @JsonIgnore | ||
| public abstract String getRouteId(); |
There was a problem hiding this comment.
Public shouldn't be needed here
|
|
||
| @Value.Immutable | ||
| public abstract class _DeleteUnmappedRoutesRequest { | ||
| /** |
| public final class CreateRouteRequestTest { | ||
|
|
||
| @Test(expected = IllegalStateException.class) | ||
| public void invalid() { |
There was a problem hiding this comment.
Should test for each missing field (seems overly detailed, but I've found errors this way in the past)
enchobelezirev
force-pushed
the
routes-v3
branch
from
0bfcc2b to
c32928f
Compare
|
any thoughts on if/when v3 routes support could be incorporated? is it in a holding pattern at the moment? thanks all for all you do EDIT^ just noticed this was added to 'todo' in cf3 15 days ago 👍 |
|
@aegershman We're currently having a push on v3 implementation, so while I don't have an exact date I'd expect it to be in the next few weeks as I'm focussing first on areas where we have PRs such as this. |
Implementation of the Routes API(https://v3-apidocs.cloudfoundry.org/version/3.77.0/index.html#routes) in the client.