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.

Sign up for GitHub

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

Jump to bottom

RSDK-9177: add billing wrappers #4570

Merged
merged 25 commits into from
Nov 26, 2024
Merged

RSDK-9177: add billing wrappers #4570

merged 25 commits into from
Nov 26, 2024

Conversation

purplenicole730
Copy link
Member

No description provided.

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 20, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 20, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
benjirewis
benjirewis reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Pretty much LGTM! Just some nits.

app/billing_client.go Show resolved Hide resolved
app/billing_client.go Outdated Show resolved Hide resolved
app/billing_client.go Outdated Show resolved Hide resolved
app/billing_client_test.go Outdated Show resolved Hide resolved
app/viam_client.go Outdated Show resolved Hide resolved
jckras
jckras reviewed Nov 22, 2024
View reviewed changes
app/billing_client.go
return data, nil
}

// SendPaymentRequiredEmail sends an email about payment requirement.
Copy link
Member

Choose a reason for hiding this comment

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

can you include how customerOrgID and billingOwnerOrgID are used in the function in the comment? I think it will make it a little more clear

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... I tried to look for the definition in the proto definition, and I can't seem to find it. It also doesn't exist in the other SDKs.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 22, 2024
jckras
jckras approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@jckras jckras left a comment

Choose a reason for hiding this comment

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

everything lgtm - the only thing we should decide on is where we should place structs in the file so we are consistent.

app/billing_client.go Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 22, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 22, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 25, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 25, 2024
benjirewis
benjirewis approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM!

@purplenicole730 purplenicole730 merged commit 07c109b into viamrobotics:main Nov 26, 2024
16 checks passed
@purplenicole730 purplenicole730 deleted the RSDK-9177-add-billing-wrappers branch November 26, 2024 14:33
randhid pushed a commit to randhid/rdk that referenced this pull request Nov 26, 2024
@purplenicole730 @randhid
RSDK-9177: add billing wrappers (viamrobotics#4570)
8a7ee51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jckras jckras jckras approved these changes

@benjirewis benjirewis benjirewis approved these changes

@njooma njooma Awaiting requested review from njooma njooma was automatically assigned from viamrobotics/sdk-netcode

Assignees
No one assigned
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@purplenicole730 @benjirewis @jckras @viambot