Create/Update route options#1339
Conversation
Refactor _RouteOptions class to handle added options in future
|
@Kehrlann Is it necessary something else to be added before code review to be done? |
|
|
||
| /** | ||
| * The loadbalancing | ||
| * All route options, including unknown future keys. |
There was a problem hiding this comment.
That's not a pattern we use anywhere, I don't think we should introduce it unless we have a very good reason to have it for this one specific sub-resource.
Otherwise, let's just add the hashes for now.
There was a problem hiding this comment.
The reason why this pattern is used is because the client library doesn't keep up well with all new features introduced in Cloudfoundry API and it's annoying that consumers of cf-java-client library cannot use latest available features until it's contributed and released. So if there is an option to make some logic more generic, I think it's better for all users. What do you think?
There was a problem hiding this comment.
This is an interesting idea, but I don't want to add it on a class-by-class basis. Is there a way to add it to our Immutables and-or JSON config?
There was a problem hiding this comment.
I have a look for some generic solution but it won't be easy to implement, so I go back to previous way of implementation by adding missing parameter options. I have added also integration test but I couldn't run it, so I hope that you can run and give back feedback is it okay.
|
@Kehrlann Do you have any other comments? Is there any issues with new integration test run? |
|
|
||
| PCF_2_13(Version.forIntegers(2, 186, 0)), | ||
|
|
||
| PCF_2_14(Version.forIntegers(2, 248, 0)), |
There was a problem hiding this comment.
As we are dealing with an v3-API it would be more apropriate to use the version number 3.183.0, which is in the same capi-release.
There was a problem hiding this comment.
we don't have version checking for v3 so I'm fine with this.
With the environments we're running, I don't think the gate matters, they're all way ahead of 2.248 (CAPI 1.198)
Lokowandtg
left a comment
Lokowandtg
left a comment
There was a problem hiding this comment.
The integration tests work and the code looks good.
|
@theghost5800 do you need this soon? Because if we target |
I am expecting this change for quite long time. So I am looking to use this feature with next release of the client library. If main branch won't be release anytime soon, please advice which branch must be targeted. |
|
Please target branch 5.x.x |
|
I have opened another PR to target branch 5.x.x - #1365 |
3 participants
Refactor _RouteOptions class to handle added options in future