-
Notifications
You must be signed in to change notification settings - Fork 0
Cluster, SVM and Aggr Feign Client #6
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a folder 'response' under 'model', we can shift it there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, will create one generic ontap response class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can be shifted to model/response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, will create one generic ontap response class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we reuse this in VolumeRequestDTO, instead of AggregateDTO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, will remove the request DTOs and use the same ontap model with null annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we reuse this in VolumeRequestDTO, instead of SvmDTO?
|
|
||
| import java.net.URI; | ||
|
|
||
| @Lazy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To instantiate only when required, we can use this across all the feign clients?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some feign client we need upfront like Cluster, SVM for others we will use @lazy
| } | ||
|
|
||
|
|
||
| @Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for response object classes, we need not to have equals, hashcode and testing methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed all responses and created one generic OnTapResponse for all object which has numRecord or records
| public void setVersion(ClusterVersion version) { | ||
| this.version = version; | ||
| } | ||
| @ApiModelProperty(value = "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these annotations were added due to swagger, and currently we need not to have them, lets remove them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it and also removed from already added files also
| this.disaggregated = disaggregated; | ||
| } | ||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add equals and hashcode using only uuid in this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
| return this; | ||
| } | ||
|
|
||
| public SvmResponse addRecordsItem(Svm recordsItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed all responses and created one generic OnTapResponse for all object which has numRecord or records
| @@ -0,0 +1,130 @@ | |||
| /* | |||
| * Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename this class to Version.java as Version class already follows standard format .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
| @@ -0,0 +1,131 @@ | |||
| /* | |||
| * Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, can we have class called response.java which accepts generic and we can remove all AggregateResponse etc response objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
| public interface AggregateFeignClient { | ||
|
|
||
| @RequestMapping(method=RequestMethod.GET) | ||
| OnTapResponse<Aggregate> getAggregateResponse(URI baseURL, @RequestHeader("Authorization") String header); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the comment for this method
| public interface SvmFeignClient { | ||
|
|
||
| @RequestMapping(method = RequestMethod.GET) | ||
| OnTapResponse<Svm> getSvmResponse(URI baseURL, @RequestHeader("Authorization") String header); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, add comments
| @FeignClient(name = "VolumeClient", url = "https://{clusterIP}/api/storage/volumes", configuration = FeignConfiguration.class) | ||
| public interface VolumeFeignClient { | ||
|
|
||
| @DeleteMapping("/storage/volumes/{id}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/api is missing
| public interface SvmFeignClient { | ||
|
|
||
| //this method to get all svms and also filtered svms based on query params as a part of URL | ||
| @RequestMapping(method = RequestMethod.GET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general comment which can be taken later. I see somewhere RequestMapping and somewhere GetMapping,PostMapping.We can have same aross all feign client
…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
Added Custer, SVM and Aggr Feign Client interfaces along with respective POJO's
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?