Skip to content
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

DE24092: exposing volume export count #132

Merged
merged 2 commits into from
Sep 23, 2024
Merged

DE24092: exposing volume export count #132

merged 2 commits into from
Sep 23, 2024

Conversation

achu-1612
Copy link
Contributor

@achu-1612 achu-1612 commented Sep 23, 2024

Description:

  • Adding ExportCount under volume schema.

Testing:

  • updated the metal client after these changes were made, in the quake vendor repo.
  • created a volume
  • edited the db recorded to populate random data for the newly added fields in the volume model.
  • used the /v1 API to test whether those fields were getting fetched.

    Screenshot 2024-09-23 131720

@achu-1612 achu-1612 marked this pull request as ready for review September 23, 2024 07:49
@chitranm chitranm requested a review from dbozzato81 September 23, 2024 07:51
Copy link
Contributor

@chitranm chitranm left a comment

Choose a reason for hiding this comment

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

Looks good to me. Deferring for approval from Mike and Denis.

@chitranm
Copy link
Contributor

Just for the context - Right now BMaaS UI is using the VolumeAttachment (not 100% sure) API to show number of exports for a given volume under volume page (as export count is not available as an attribute in project API response). This works well for the Metal managed volumes but not for imported volumes.

Addition of this new attribute will eliminate the usage of VolumeAttachment API to get the number of exports on a volume and will help to show export count in case of imported volumes.

required:
- ActiveSite
- Capacity
- CapacityUsed
- CreatedSite
- Description
- ExportCount

Choose a reason for hiding this comment

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

why ExportCount is mandatory for unexported volumes?

Copy link
Contributor Author

@achu-1612 achu-1612 Sep 23, 2024

Choose a reason for hiding this comment

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

As this schema is only used for response not any request,
We will get this value (populated or 0 initialized value) from server. I though it was okay to add it as a req field.
Will remove this as required field

Copy link
Contributor

Choose a reason for hiding this comment

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

If we expect this attribute to always be sent from backend it needs to be marked as required

@@ -55,4 +55,6 @@ type Volume struct {
ActiveSite string `json:"ActiveSite"`
// The site where the volume was originally created.
CreatedSite string `json:"CreatedSite"`
// The number of active exports for this volume
ExportCount int32 `json:"ExportCount"`

Choose a reason for hiding this comment

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

Is there any reason not consistent with naming convention as in portal API model?
https://github.com/hpe-hcss/quake/blob/dd3d55c5afc9a4a7bb76f56daded9e0c3dd135cc/model/volume.go#L170

In portal api models, field name is like this:
json:"export_cnt"

Copy link
Contributor Author

@achu-1612 achu-1612 Sep 23, 2024

Choose a reason for hiding this comment

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

Yes, but as this will be exposed to end-users.
I thought it is better not to name the variable in short form.
let me know if you want me to change it from ExportCount -> ExportCnt

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @harinathakotha, AFAIK the only convention we have for Project API is camel case. Both ExportCount and ExportCnt are fine.

Copy link
Contributor

@dbozzato81 dbozzato81 left a comment

Choose a reason for hiding this comment

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

LGTM

required:
- ActiveSite
- Capacity
- CapacityUsed
- CreatedSite
- Description
- ExportCount
Copy link
Contributor

Choose a reason for hiding this comment

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

If we expect this attribute to always be sent from backend it needs to be marked as required

@achu-1612 achu-1612 merged commit 8a5b529 into master Sep 23, 2024
2 checks passed
@achu-1612 achu-1612 deleted the DE24092 branch September 23, 2024 14:44
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