Feignconfiguration and volume feignClient along with desired POJOs with cstack 28 #4
Conversation
rajiv-jain-netapp
requested review from
piyush5netapp,
sandeeplocharla and
suryag1201
| <artifactId>cloud-engine-storage-volume</artifactId> | ||
| <version>${project.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
for now yes, we can remove it later if required.
...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeSpace.java
Outdated
Show resolved
Hide resolved
...orage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/FeignConfiguration.java
Show resolved
Hide resolved
.../volume/ontap/src/main/resources/META-INF/cloudstack/storage-volume-ontap/logback-spring.xml
Show resolved
Hide resolved
plugins/storage/volume/ontap/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.json</groupId> | ||
| <artifactId>json</artifactId> | ||
| <version>20230227</version> |
There was a problem hiding this comment.
It is better to keep version under properties tag as it is easy to maintain. Same is applicable for other dependencies
| private List<AggregateDTO> aggregates; | ||
| private SvmDTO svm; | ||
|
|
||
| // getters and setters |
There was a problem hiding this comment.
I am trying to keep dependencies less. Hence prefer to write getter and setter manually
There was a problem hiding this comment.
There is one caveat with @DaTa , it writes its own equals and hashcode method by comparing all fields of object which we dont want sometimes.
|
|
||
| public static class Job { | ||
| private String uuid; | ||
| private Links links; |
There was a problem hiding this comment.
same here, use @DaTa annotation for other classes also
There was a problem hiding this comment.
Same as above response
| public void setName(String name) { this.name = name; } | ||
| } | ||
|
|
||
| public static class SvmDTO { |
There was a problem hiding this comment.
we should keep this DTO outside bcz it can be reusable by other DTOs also
There was a problem hiding this comment.
I thought of keeping them inside for now and taking them out case by case. That way, we can keep the count of files lower. The number of files is already shooting up.
rajiv-jain-netapp
left a comment
rajiv-jain-netapp
left a comment
There was a problem hiding this comment.
responded on the comments, reposting the changes with the new changes as per comments
| <properties> | ||
| <spring-cloud.version>2021.0.7</spring-cloud.version> | ||
| <openfeign.version>11.0</openfeign.version> | ||
| </properties> |
There was a problem hiding this comment.
Need to include both spring-web and spring-boot-starter(v2.7.10), if we intend to use @FeignClient annotation in VolumeFeignClient.java
There was a problem hiding this comment.
As mentioned earlier, I don't see any compilation issues with the newly added code. If additional dependencies are required as we proceed, we can incorporate them accordingly. For now, I'd prefer not to include all potential dependencies until the corresponding code is in place.
rajiv-jain-netapp
left a comment
rajiv-jain-netapp
left a comment
There was a problem hiding this comment.
reverted to the comments
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class Aggregate { | ||
|
|
||
| @SerializedName("name") |
There was a problem hiding this comment.
I see in some model we are using JsonProperty and SerializedName which serves same purpose but are from 2 different libraries. What's your thought on using Jackson or gson any one of the above across our models.
| private List<AggregateDTO> aggregates; | ||
| private SvmDTO svm; | ||
|
|
||
| // getters and setters |
There was a problem hiding this comment.
There is one caveat with @DaTa , it writes its own equals and hashcode method by comparing all fields of object which we dont want sometimes.
…ommits. # This is the 1st commit message: CSTACKEX-25: Basic class structure # This is the commit message #2: Add PrimaryStoragePool base code # This is the commit message #3: CSTACKEX-25: Create Volume code basic code added # This is the commit message #4: CSTACKEX-25: additional logic for Primary storage pool creation # This is the commit message #5: CSTACKEX-29 Cluster, SVM and Aggr Feign Client # This is the commit message #6: CSTACKEX-29 Added License Info # This is the commit message #7: CSTACKEX-29 Resolve Review Comments # This is the commit message #8: CSTACKEX-29 Resolve Style check issues � This is the commit message #9: CSTACKEX-29 Resolve Style check issues � This is the commit message #10: CSTACKEX-29 Resolve Precommits Issues # This is the commit message #11: CSTACKEX-29 Resolve Precommits Issues
Description
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?