Conversation
olivergondza
left a comment
olivergondza
left a comment
There was a problem hiding this comment.
The code looks good to me in general. I have added several comments where is can be improved, they are mostly cosmetic.
core-test/src/main/java/org/openstack4j/api/network/TrunkTests.java
Outdated
Show resolved
Hide resolved
core-test/src/main/java/org/openstack4j/api/network/TrunkTests.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/openstack4j/openstack/networking/domain/NeutronPort.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/openstack4j/openstack/provider/DefaultAPIProvider.java
Show resolved
Hide resolved
| /** | ||
| * @return a list of subports associated with the trunk | ||
| */ | ||
| List<NeutronSubPort> getSubPorts(); |
There was a problem hiding this comment.
The interface method should not be returning an implementation, type but its interface instead.
|
Hi Oliver, |
olivergondza
left a comment
olivergondza
left a comment
There was a problem hiding this comment.
Thanks. I have made a second round of review.
core/src/main/java/org/openstack4j/api/networking/NetworkingService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/openstack4j/api/networking/TrunkService.java
Outdated
Show resolved
Hide resolved
| * the ID of the trunk to update | ||
| * @return | ||
| */ | ||
| Trunk updateTrunk(Trunk trunk, String trunkId); |
There was a problem hiding this comment.
I am not quire familiar with the OpenStack trunk service, but the contract of this method got my attention. It would be cleaner if the trunkId is passed in inside the trunk. Is there any reason for not doing it that way?
core/src/main/java/org/openstack4j/api/networking/TrunkService.java
Outdated
Show resolved
Hide resolved
|
Hi Oliver, |
|
Great, did you have a chance to have a look at #52 (comment)? |
|
Oh sorry I totally forgot to reply to that one. So basically I just followed how the existing resources do it to maintain consistency. |
|
@kashyapjha, you are correct this is being done that way sometimes, but it is generally inconsistent all over the codebase. But there are cases when this is done the simple way[1]. Also the method conventions suggest to use name [1] |
|
Also the updateTrunk method takes in a builder built trunk object to update that's why the id is passed in separately |
|
@kashyapjha, that is my point exactly. We wrap 7 input values in one argument, but we pass the 8th separately. Is there a reason for the ID not being part of the builder and entity? |
|
The ID is used to retrieve the current trunk object from openstack and the trunk object which can have all attributes except ID is passed in as the object to update because we cant/shouldnt change a resource ID |
|
Yes, I understand that's what you are proposing. Though what I suggest is to change that to be aligned with other implementations providing cleaner API for clients. The ID in the [1] |
|
Okay I see what you mean. I'll try to implement that way and let you know if it's possible. |
|
Hi Oliver, I've updated it. |
olivergondza
left a comment
olivergondza
left a comment
There was a problem hiding this comment.
Great, thanks. I will consult the test results and merge towards the next release.
|
@kashyapjha, I have no idea why, but travis needed a manual rebuild to report test results here. Unfortunately, there are some failing tests. |
|
I'm sorry I got a mess of repos and branches so I made a new pull request. All changes you requested are there and unit tests are passing as well |
|
Closing in favor of #53. |
2 participants
No description provided.