FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Cluster, SVM and Aggr Feign Client by suryag1201 · Pull Request #6 · NetApp/cloudstack · GitHub

Cluster, SVM and Aggr Feign Client - #6

Merged
suryag1201 merged 10 commits into
mainfrom
feature/CSTACKEX-29
Oct 16, 2025
Merged

Cluster, SVM and Aggr Feign Client#6
suryag1201 merged 10 commits into
mainfrom
feature/CSTACKEX-29

Conversation

suryag1201 commented Oct 15, 2025
edited
Loading

Copy link
Copy Markdown

Description

Added Custer, SVM and Aggr Feign Client interfaces along with respective POJO's

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There's a folder 'response' under 'model', we can shift it there

suryag1201 Oct 15, 2025
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As discussed, will create one generic ontap response class

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This file can be shifted to model/response

suryag1201 Oct 15, 2025
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As discussed, will create one generic ontap response class

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can't we reuse this in VolumeRequestDTO, instead of AggregateDTO?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As discussed, will remove the request DTOs and use the same ontap model with null annotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can't we reuse this in VolumeRequestDTO, instead of SvmDTO?


import java.net.URI;

@Lazy

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To instantiate only when required, we can use this across all the feign clients?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

some feign client we need upfront like Cluster, SVM for others we will use @lazy

}


@Override

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

for response object classes, we need not to have equals, hashcode and testing methods.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 = "")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

these annotations were added due to swagger, and currently we need not to have them, lets remove them

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

removed it and also removed from already added files also

this.disaggregated = disaggregated;
}
@Override
public String toString() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

we can add equals and hashcode using only uuid in this class

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done!

return this;
}

public SvmResponse addRecordsItem(Svm recordsItem) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

remove this

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can rename this class to Version.java as Version class already follows standard format .

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done!

@@ -0,0 +1,131 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just a thought, can we have class called response.java which accepts generic and we can remove all AggregateResponse etc response objects.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done!

public interface AggregateFeignClient {

@RequestMapping(method=RequestMethod.GET)
OnTapResponse<Aggregate> getAggregateResponse(URI baseURL, @RequestHeader("Authorization") String header);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Add the comment for this method

public interface SvmFeignClient {

@RequestMapping(method = RequestMethod.GET)
OnTapResponse<Svm> getSvmResponse(URI baseURL, @RequestHeader("Authorization") String header);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

same here, add comments

@@ -34,27 +37,20 @@
public interface VolumeFeignClient {

@DeleteMapping("/storage/volumes/{id}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

/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)

piyush5netapp Oct 16, 2025
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A general comment which can be taken later. I see somewhere RequestMapping and somewhere GetMapping,PostMapping.We can have same aross all feign client

piyush5netapp self-requested a review October 16, 2025 04:59
suryag1201 merged commit f7b837a into main Oct 16, 2025
16 checks passed
sandeeplocharla pushed a commit that referenced this pull request Oct 21, 2025
…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
piyush5netapp pushed a commit that referenced this pull request Dec 4, 2025
# This is the 1st commit message:

NFS Cloudstack volume and export policy utils

# This is the commit message #2:

Licencse add in files

# This is the commit message #3:

accessgroup create recode

# This is the commit message #4:

creatacessgroup for NFS impl

# This is the commit message #5:

storage pool mounting on host

# This is the commit message #6:

storage pool mounting on host 1

# This is the commit message #7:

vm restart issue

# This is the commit message #8:

vm restart issue 1

# This is the commit message #9:

vm restart issue 2

# This is the commit message #10:

vm instance creation test1

# This is the commit message #11:

vm instance creation test4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL