-
Notifications
You must be signed in to change notification settings - Fork 4.5k
api: added helper types for census reporting ent api #22837
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
da7dc83 to
8d4633b
Compare
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.
Added few minor comments.
| if q != nil { | ||
| r.setQueryOptions(q) | ||
| } | ||
| if req != nil { |
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.
IMO, the request must not be optional. return appropriate error
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 per the CLI, all the flags are optional, i.e. each of message, today-only and send_report will have a default value. Therefore, a GET request without any query params is valid and an empty request should be handled with default value.
As for CustomerID, it is a required parameter for CreateBundle but it can be derived later in reporting package itself using license data. Hence, I have removed it from here.
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.
What is the rational behind passing the Customer ID in CLI. This information is already available in License and hence it can be bundled in the response.
api/operator_utilization.go
Outdated
| r.setQueryOptions(q) | ||
| } | ||
| if req != nil { | ||
| if req.CustomerID != "" { |
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.
IMO, the CustomerID must not be optional. return error
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 this as a request param as it will be derived in reporting package using the license data at the time of starting reporting agent for metric collection. This CustomerID will be maintained in cache there after for Bundle creation.
api/txn.go
Outdated
| type CensusOp string | ||
|
|
||
| const ( | ||
| CensusPut CensusOp = "put" |
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 you take care of linting issue.
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.
Fixed the lint errors now
8d4633b to
968ce8d
Compare
968ce8d to
97a28ca
Compare
97a28ca to
885fecc
Compare
885fecc to
4f8a186
Compare
4f8a186 to
ef57b99
Compare
ef57b99 to
7052cd6
Compare
srahul3
left a comment
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.
✅ LGTM
| if q != nil { | ||
| r.setQueryOptions(q) | ||
| } | ||
| if req != nil { |
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.
What is the rational behind passing the Customer ID in CLI. This information is already available in License and hence it can be bundled in the response.
| // Utilization generates a census utilization bundle by calling the | ||
| // /v1/operator/utilization endpoint. The returned byte slice contains the bundle | ||
| // JSON payload suitable for saving to disk or further processing. | ||
| func (c *Census) Utilization(req *UtilizationBundleRequest, q *QueryOptions) ([]byte, *QueryMeta, error) { |
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.
Please add the response struct method in followup PR.
|
📣 Hi @sujay-hashicorp! a backport is missing for this PR [22837] for versions [1.18,1.19] please perform the backport manually and add the following snippet to your backport PR description: |
1 similar comment
|
📣 Hi @sujay-hashicorp! a backport is missing for this PR [22837] for versions [1.18,1.19] please perform the backport manually and add the following snippet to your backport PR description: |
Description
Added Census client for handling operator utilization related calls
Testing & Reproduction steps
n/a
Links
n/a
PR Checklist
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.