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

feat(platform): add payment cli for streaming payments w/ ApePay #129

Merged
merged 15 commits into from
Oct 9, 2024

Conversation

fubuloubu
Copy link
Member

@fubuloubu fubuloubu commented Oct 7, 2024

What I did

In anticipation for our beta launch, we give people the ability to fund their Clusters using the Silverback CLI

This PR adds support for funding newly created Clusters, adding more funds to extend the time of those Clusters, and also cancelling those Clusters via CLI

How I did it

How to verify it

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
    - [ ] New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@fubuloubu fubuloubu marked this pull request as ready for review October 8, 2024 00:31
docs/userguides/platform.md Show resolved Hide resolved
silverback/_cli.py Outdated Show resolved Hide resolved
silverback/cluster/client.py Outdated Show resolved Hide resolved
@@ -20,6 +20,23 @@ The Platform UI will let you create and manage Clusters using a graphical experi
The CLI experience is for those working locally who don't want to visit the website, or are locally developing their applications.
```

Once you have created your Cluster, you have to fund it so it is made available for your use.
To do that, use the [`silverback cluster pay create`][silverback-cluster-pay-create] command to fund your newly created cluster.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion(non-blocking): I wonder if it's worth mentioning that you need a funded Ape account for this to work. Might confuse things maybe, but I'm not sure if all Silverback users will have an Ape environment ready to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion!

Yes, the UI will be more hand-holdy through this experience, only so much I can do via CLI without assuming a certain level of experience from the user

@@ -20,6 +20,23 @@ The Platform UI will let you create and manage Clusters using a graphical experi
The CLI experience is for those working locally who don't want to visit the website, or are locally developing their applications.
```

Once you have created your Cluster, you have to fund it so it is made available for your use.
To do that, use the [`silverback cluster pay create`][silverback-cluster-pay-create] command to fund your newly created cluster.
Please note that provisioning your cluster will take time, and it may take up to an hour for it to be ready.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I think it's safe to lower that expectation a bit to no more than 30 minutes. I can't seem to find my initial reference for some reason, but I thought the AWS docs mentioned up to 15 minutes. And in my experience it hasn't taken that long.

Did you see longer or something?

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 leaving space in case we have something slowing it down. In practice I see just under 10 minutes.

But also, the ApePay stream cannot be touched until after 1 hour, which is sort of the hard limit for safety purposes (not allow more changes to the stream parameters while infra changes are occuring)

We can move these parameters inwards later

Copy link
Member

Choose a reason for hiding this comment

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

Just seems like we're shouting at the user that "this will be so incredibly slow you'll grow a beard" before they even get to try it. Though at least they'll be pleasantly surprised with a 10 minutes deploy. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the opposite, if you don't explicitly call out that it is going to take longer than like ~5 seconds or so, then it might be confusing why you can't use it immediately, and it's great to share that expectation early

This seems more like a UX expectation thing, so I'll let @joshcoppola give his opinion

Choose a reason for hiding this comment

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

I agree with @fubuloubu that it's better to be honest that things will take some time rather than leave a user confused that they can't instantly use the cluster.

How about something like "Provisioning a cluster usually takes 10 minutes, but could take as long as an hour"? That communicates the expected time while still giving some buffer if things do take way longer than we expect.

Then we could also follow up with something like "For safety purposes, ApePay streams are always locked for one hour" to communicate that side of things (I'm out of my element on how to phrase that but you get the idea)

Copy link
Member

Choose a reason for hiding this comment

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

I sure never said lie. I was just suggesting make it more in line with reality.

If you want to set expectations to be like 6x what we expect then to be then so be it. But it's not like we've seen an hour long deploy though. And the cancellation window has nothing to do with how long the deployment would take.

Choose a reason for hiding this comment

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

I didn't mean to imply you were advocating for lying, I meant to agree with @fubuloubu 's point about sharing the time expectation with users.

I don't have an informed opinion on exactly how much buffer time would be good to communicate, mainly I wanted to point out that we could include the actual expected time and another value with some buffer time in the message.

Even though it is separate from cluster deployment, I think it's worth mentioning the ApePay cancellation window somewhere if we don't already call that out somewhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can fuse it all together and say the expectation is 10 minutes, but it may take longer, and you can't cancel it until an hour has passed.

mikeshultz
mikeshultz previously approved these changes Oct 8, 2024
Copy link
Member

@mikeshultz mikeshultz left a comment

Choose a reason for hiding this comment

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

Rest of my feedback and open discussions are non-blocking. LGTM.

@johnson2427 johnson2427 enabled auto-merge (squash) October 9, 2024 13:49
@johnson2427
Copy link
Contributor

@mikeshultz I'm blocked from being able to approve this PR, need your grace on this

@johnson2427 johnson2427 merged commit e4925e3 into main Oct 9, 2024
22 checks passed
@johnson2427 johnson2427 deleted the feat/payment-cli branch October 9, 2024 16:17
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.

5 participants