add Invoices table #122
add Invoices table #122
Conversation
|
@misraved why did you assigned me the PR ? Maybe you have "forgot" the corresponding comment ? |
There was a problem hiding this comment.
Hi @tdannenmuller, I’ve shared some initial review comments—please take a look. Thank you!
Additionally, here are a few key points to address:
- Update the table name to
scaleway_billing_invoiceand make corresponding updates to the function name and file name. - Remove any unnecessary log statements.(Typically, we avoid including
DebugorWarnstatements unless they are absolutely necessary.)
| } | ||
| } | ||
|
|
||
| func extractAmount(ctx context.Context, d *transform.TransformData) (interface{}, error) { |
There was a problem hiding this comment.
Could you please rearrange the functions in the following order?
- List Function
- Get Function
- Other hydrate Function
- Transform Function
- Utility Function
For reference: https://github.com/turbot/steampipe-plugin-aws/blob/main/aws/table_aws_acm_certificate.go
| } | ||
|
|
||
| func extractAmount(ctx context.Context, d *transform.TransformData) (interface{}, error) { | ||
| plugin.Logger(ctx).Debug("extractAmount", "field", d.ColumnName, "raw_value", d.Value) |
There was a problem hiding this comment.
| plugin.Logger(ctx).Debug("extractAmount", "field", d.ColumnName, "raw_value", d.Value) |
| plugin.Logger(ctx).Debug("extractAmount", "field", d.ColumnName, "raw_value", d.Value) | ||
|
|
||
| if d.Value == nil { | ||
| plugin.Logger(ctx).Warn("extractAmount: nil value", "field", d.ColumnName) |
There was a problem hiding this comment.
| plugin.Logger(ctx).Warn("extractAmount: nil value", "field", d.ColumnName) |
| } | ||
|
|
||
| for _, invoices := range resp.Invoices { | ||
| plugin.Logger(ctx).Debug("raw invoices data", "invoices", fmt.Sprintf("%+v", invoices)) |
There was a problem hiding this comment.
| plugin.Logger(ctx).Debug("raw invoices data", "invoices", fmt.Sprintf("%+v", invoices)) |
| } | ||
|
|
||
| // Make the API request to list invoices | ||
| resp, err := billingAPI.ListInvoices(req) |
There was a problem hiding this comment.
Could you please add support for API pagination for this API?
According to the API documentation, the ListInvoices API supports pagination. For implementing this feature, you may refer to the implementation in the scaleway_account_project table.
| Name: "seller_name", | ||
| Type: proto.ColumnType_STRING, | ||
| Description: "The name of the seller.", | ||
| }, |
There was a problem hiding this comment.
Could you please include the Scaleway standard columns and Steampipe standard columns in this table to maintain consistency across all tables?
| organizationID = *scalewayConfig.OrganizationID | ||
| } | ||
|
|
||
| // Check if organization_id is specified in the query |
There was a problem hiding this comment.
| // Check if organization_id is specified in the query | |
| // Check if organization_id is specified in the query parameter |
|
|
||
| // Log a warning if organization_id is not provided by either config or query | ||
| if organizationID == "" { | ||
| plugin.Logger(ctx).Warn("scaleway_invoices.listScalewayInvoices", "warning", "No organization_id provided in config or query") |
There was a problem hiding this comment.
It might be more appropriate to return an error instead of just a warning message in this scenario.
Could you please clarify how the API behaves when the Organization ID is not provided?
Does it default to returning results for the current Organization associated with the provided access key and secret key?
If the API does not return an error in such cases, I kindly suggest removing this block of code for simplicity and consistency with the API behavior.
| KeyColumns: []*plugin.KeyColumn{ | ||
| {Name: "organization_id", Require: plugin.Optional}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Kindly include the Get Config as well, allowing users to retrieve details for a single invoice. This addition could help optimize query performance and improve efficiency.
| } | ||
| } | ||
|
|
||
| type invoicesItem struct { |
There was a problem hiding this comment.
Is it necessary to define this structure explicitly?
|
Hello, I'm a bit confused. I thought you were waiting for the changes to merge. For the record, I've started to implement you're comments, but i've not finished yet. I have several questions / remarks concerning them, but it is still a work in progress. I'll try to take some time soon to do so. How should we proceed in order to bring those future changes to the main repo ? Thx. |
|
Apologies for any confusion, @tdannenmuller. Since we hadn’t heard from you in a while and the overall changes in the PR looked great, we went ahead and merged it into a branch. I’ve also made some cleanup changes and raised a follow-up PR (#123) with Please feel free to continue adding your updates to the Thanks! |
I've added support for a new table named invoices. This table recover the data form : https://www.scaleway.com/en/developers/api/billing/#path-invoices-list-invoices using scaleway-sdk-go/api/billing
I'm not a go developer, so I've intensively used AI to generate the table code. I've tried my best to conform to coding convention and styling of the project.
The table have been extensively tested, and the documentation reflects that.
I humbly hope this contribution is decent enough to be integrated.