Add "total_service_keys" to quota json unmarshalling #2881
Conversation
There was a problem hiding this comment.
Thank you for your contribution to CLI. Looks like this change needs to be applied at other places like quota_resource_test.go and space and org quota tests in different packages and several other non test files. Can you please update the PR with these changes?
The same goes for other PRs as well
| @@ -71,6 +71,7 @@ func (al *AppLimit) UnmarshalJSON(rawJSON []byte) error { | |||
| type ServiceLimit struct { | |||
| TotalServiceInstances *types.NullInt `json:"total_service_instances,omitempty"` | |||
| PaidServicePlans *bool `json:"paid_services_allowed,omitempty"` | |||
| TotalServiceKeys *types.NullInt `json:"total_service_keys,omitempty"` | |||
| } | |||
|
|
|||
| func (sl *ServiceLimit) UnmarshalJSON(rawJSON []byte) error { | |||
There was a problem hiding this comment.
question Do we need some custom logic + test to handle the nil case? Seems like we have that already for TotalServiceInstances. See https://github.com/cloudfoundry/cli/pull/2881/files#diff-305a3cec50fa59b749c31f4599960a65a465a66bfca7afe1ccce4fcb58fad06eL87-L92
There was a problem hiding this comment.
With this in mind, I'm curious to see the results of min_capi tests when they will get fixed.
|
LGTM, two quick questions I had is that while this change adds the definition to the go-lang structs, this does not add them to any CLI output https://github.com/cloudfoundry/cli/blob/main/command/v7/shared/quota_displayer.go#L57-L66 Firstly, @sleungcy-sap I want to double check that was the intention - this is just a change in the CLI library for consumers of the go module right? Secondly (non-blocking on this MR), @a-b / @gururajsh if we have this in the struct - should we be adding this to the CLI output? The column output is treated like an API contract right, is there a process for adding new columns (e.g. can we just add a new column on the end or is this considered a breaking change)? |
|
@Samze Yes, this change is for consumers of this repo as a go module |
|
@sleungcy-sap we discussed this on the WG call this week. While the cli is listed as an experimental client, ideally we would not add functionality to the cli library without exposing the feature in the UX of the CLI itself. As an alternative have you considered using https://github.com/cloudfoundry/go-cfclient which is now listed as the If you are interested in expanding the PR to include contributions for UX changes, then we can discuss the next steps for that. |
|
Closing this PR for now, feel free to create an issue that includes a proposal for the UX changes. |
Description of the Change
Consumers of this library can access the total_service_keys field from the api with this change.
Why Is This PR Valuable?
The value of this parameters is not included into unmarshalled data and is missing at the moment. The value should be available from the API as documented in https://v3-apidocs.cloudfoundry.org/version/3.163.0/#organization-quotas-in-v3