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

RSDK-4907 - Add billing client #471

Merged
merged 17 commits into from
Oct 26, 2023
Merged

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Oct 25, 2023

Major changes

Adds billing client to python SDK.

Comment on lines +881 to +897
async def GetBillingSummary(self, stream: Stream[GetBillingSummaryRequest, GetBillingSummaryResponse]) -> None:
raise NotImplementedError()

async def GetCurrentMonthUsageSummary(
self,
stream: Stream[GetCurrentMonthUsageSummaryRequest, GetCurrentMonthUsageSummaryResponse],
) -> None:
raise NotImplementedError()

async def GetInvoiceHistory(self, stream: Stream[GetInvoiceHistoryRequest, GetInvoiceHistoryResponse]) -> None:
raise NotImplementedError()

async def GetItemizedInvoice(self, stream: Stream[GetItemizedInvoiceRequest, GetItemizedInvoiceResponse]) -> None:
raise NotImplementedError()

async def GetUnpaidBalance(self, stream: Stream[GetUnpaidBalanceRequest, GetUnpaidBalanceResponse]) -> None:
raise NotImplementedError()
Copy link
Member Author

Choose a reason for hiding this comment

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

These methods are slated to be deprecated and so are not being implemented.

assert service.org_id == org_id

@pytest.mark.asyncio
async def test_get_invoice_pdf(self, service: MockBilling):
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewers: the client code for get_invoice_pdf saves the pdf to a file. I didn't really want to mess around with having a test in CI that creates and modifies files, and the only other alternative I could think of (make the filepath optional, return the pdf's byte array instead of nothing, and then compare to that return value) I think would be a bad API change to the get_invoice_pdf method (the byte array is not particularly human parse-able or useful outside of being saved to a pdf). In local testing I was able to download and save invoice PDFs successfully so I figured that was good enough. If there's any disagreement with the above, let me know!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable to me

@stuqdog stuqdog marked this pull request as ready for review October 25, 2023 18:29
@stuqdog stuqdog requested a review from a team as a code owner October 25, 2023 18:29
response: GetInvoicePdfResponse = await self._billing_client.GetInvoicePdf(request, metadata=self._metadata, timeout=timeout)
data: bytes = response[0].chunk
file = open(dest, "wb")
file.write(data)
Copy link
Member

@dgottlieb dgottlieb Oct 25, 2023

Choose a reason for hiding this comment

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

This should be followed with a file.flush().

And probably an os.fsync(file), but that's less likely to prevent a surprising behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh good callout, thanks! Also adding file.flush() to other app client methods that write to file.

Copy link
Member

Choose a reason for hiding this comment

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

Post-approve realization mentioned in person: We should be file.close()ing here, or with open(...) as file:. Which omits the need for a flush here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just switched to with open

@stuqdog stuqdog requested a review from dgottlieb October 25, 2023 18:48
Copy link
Member

@dgottlieb dgottlieb left a comment

Choose a reason for hiding this comment

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

still lgtm

@stuqdog stuqdog merged commit 8d1b673 into viamrobotics:main Oct 26, 2023
9 checks passed
@stuqdog stuqdog deleted the add-billing-client branch October 26, 2023 14:49
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.

2 participants