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

chore: Swapping Postman with Bruno #2422

Merged
merged 17 commits into from
Feb 29, 2024
Merged

chore: Swapping Postman with Bruno #2422

merged 17 commits into from
Feb 29, 2024

Conversation

sabineschaller
Copy link
Member

@sabineschaller sabineschaller commented Feb 19, 2024

Changes proposed in this pull request

  • get rid of postman assets
  • add bruno assets

Context

Postman keeps giving us sync issues. Bruno's version control is pure git, the files are readable, the UI is good. No paid subscription required anymore.
Also, Bruno's scripting is more powerful. I've only made our old scripts work with Bruno but technically, the lambda function shouldn't be required anymore, since Bruno can import node packages. In a follow-up PR, I'll get rid of the lambda.

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Postman collection updated

Copy link

netlify bot commented Feb 19, 2024

Deploy Preview for brilliant-pasca-3e80ec ready!

Name Link
🔨 Latest commit b7708c1
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65e05d91a539780008ceaa21
😎 Deploy Preview https://deploy-preview-2422--brilliant-pasca-3e80ec.netlify.app/playground/overview
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sabineschaller sabineschaller changed the title Swapping Postman with Bruno chore: Swapping Postman with Bruno Feb 19, 2024
@github-actions github-actions bot added the type: ci Changes to the CI label Feb 19, 2024
@sabineschaller
Copy link
Member Author

I am going to rename the "postman" subdirectory as soon as people are happy with this PR.

mkurapov
mkurapov previously approved these changes Feb 19, 2024
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

Looks awesome 🎉 thank you for this! Everything worked for me, after I figured out how to set clientPrivateKey in the secrets.

Writing down a few things for next PR to not forget:

  • We should enable prettier to be able to prettify the scripts.js file
  • We can remove the Postman github actions
  • We should update the docs to replace any references to Postman

@github-actions github-actions bot added type: localenv Local playground pkg: mock-ase pkg: documentation Changes in the documentation package. type: documentation (archived) Improvements or additions to documentation labels Feb 22, 2024
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

nice new excalidraw picture btw

localenv/README.md Outdated Show resolved Hide resolved

1. run `docker logs rafiki-cloud-nine-mock-ase-1`
2. find the list of created wallet addresses
3. copy the url of one of the wallet addresses
4. set the url into `senderWalletAddress` postman variable in `Remote Environment`
4. set the url as `senderWalletAddress` variable in the Bruno `Remote` environment
Copy link
Contributor

Choose a reason for hiding this comment

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

should we mention the clientPrivateKey step in 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.

Yes, good catch!

Copy link
Contributor

@BlairCurrey BlairCurrey Feb 22, 2024

Choose a reason for hiding this comment

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

Couldn't we just pre-configure it according to the instructions instead of having these instructions? If we're suggesting to use the remote environment with a local wallet address then maybe we should just change https://ilp.rafiki.money/d99f0eee to http://localhost:3000/accounts/gfranklin?

Or are we running bruno from some other context where we want to keep that remote wallet address as it is? Even still I imagine this use-case is basically the default.

I guess I'm also wondering why we want to point to the remote environment in the context of local development? I mean I get it in terms of integrating but for our development purposes why use remote?

I feel like these steps could basically be "load the collection from this path" and "select the local environment".

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we add an autopeering environment in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sabineschaller that's a good idea 👍

localenv/README.md Outdated Show resolved Hide resolved

The Open Payments APIs can be interacted with using the Postman collection ([Open Payments APIs](https://www.postman.com/interledger/workspace/interledger/folder/22855701-1b204bc1-c8e5-44d4-bab9-444d7204b15a?ctx=documentation) and [Open Payments Auth APIs](https://www.postman.com/interledger/workspace/interledger/folder/22855701-ae80b96d-4d25-42b9-94fa-8ed17f0e5ed9?ctx=documentation)). It is configured to use the default endpoints of the local environment.
The Open Payments APIs can be interacted with using the [Bruno collection](https://github.com/interledger/rafiki/main/bruno/collections/Interledger) and its ([Open Payments APIs](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/Open%20Payments%20APIs) and [Open Payments Auth APIs](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/Open%20Payments%20Auth%20APIs)). It is configured to use the default endpoints of the local environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

When I look at this without all the green highlighted URLs, it reads like this: "The Open Payments APIs can be interacted with using the Bruno collection and its Open Payments APIs and Open Payments Auth APIs." Are you saying that Bruno has its own set of Open Payments APIs that can interact with some other set of Open Payments APIs? In short, it seems repetitive, but I'm not sure if that's intentional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "The Bruno collection can be used to interact with the Open Payments API and Open Payments Auth API"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with that description, but I think I asked @sabineschaller during the Rafiki work week whether these were all separate APIs or just a single Open Payments API and, I think she said single..? Sabine can you refresh my memory?

Copy link
Member Author

@sabineschaller sabineschaller Feb 28, 2024

Choose a reason for hiding this comment

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

Thank you for making me be more precise @melissahenderson
We technically have 3 Open Payments APIs now:

  • resource server API
  • auth server API
  • wallet address API

I don't think we make the distinction between resource server and wallet address API in the collection yet, because we have not moved the wallet address to be hosted on its own server.

Nevertheless, I think the correct wording would be

The Open Payments APIs can be interacted with using the Bruno collection (resource server endpoints and auth server endpoints).

localenv/README.md Outdated Show resolved Hide resolved

The Examples folder in the Postman API collection includes an eCommerce (Open Payments) example that can be executed one by one. It
1. load the collection into Bruno by clicking "Open Collection"
2. navigating to `/rafiki/main/bruno/collections/Interledger` on your machine and
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call the collection "Rafiki" instead?

Comment on lines 143 to 145
2. navigating to `/rafiki/main/bruno/collections/Interledger` on your machine and
3. clicking "Open"
Furthermore, you need to either load the [Local Environment](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/environments/Local%20Playground.bru) or the [Remote Environment](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/environments/Remote.bru).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
2. navigating to `/rafiki/main/bruno/collections/Interledger` on your machine and
3. clicking "Open"
Furthermore, you need to either load the [Local Environment](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/environments/Local%20Playground.bru) or the [Remote Environment](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/environments/Remote.bru).
2. navigating to `/rafiki/bruno/collections/Interledger` on your machine and clicking "Open"
3. Furthermore, you need to either load the [Local Environment](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/environments/Local%20Playground.bru) or the [Remote Environment](https://github.com/interledger/rafiki/main/bruno/collections/Interledger/environments/Remote.bru).

Should it be like this instead?

@@ -0,0 +1,133 @@
const fetch = require('node-fetch')
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way we can maybe have this file in the root folder so that we don't have the empty "scripts" folder in the collection?

Comment on lines +14 to +15
vars:secret [
clientPrivateKey
Copy link
Contributor

Choose a reason for hiding this comment

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

is the consensus in the end to keep this secret, even on local playground?

Copy link
Contributor

@BlairCurrey BlairCurrey Feb 28, 2024

Choose a reason for hiding this comment

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

If so I think we still need to add some instruction on where to get it (didn't see this added anywhere in the latest commits). I'm still confused about that actually. I pulled the one from postman which worked, and I thought maybe I could generate a key and copy the base64 version from rafiki.money but that doesnt work. I suppose even if we hardcoded one that works (which I'm still a proponent of since its not actually sensitive and it reduces setup, but not gonna insist on it) we should have some documentation of where this comes from.

6. creates a quote on the sender's account
7. requests a grant to create (and read) an outgoing payment on the sender's account
8. creates an outgoing payment on the sender's account
9. fetches the outgoing payment on the sender's account
8. continues the grant request (via the interaction flow)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just say about copying over the interaction_ref?

What about changing the outgoing payment grant request altogether to not have a finish method, and just be able to call continue request without needing to enter the interaction_ref using the new polling method?

mkurapov
mkurapov previously approved these changes Feb 28, 2024
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

Actually, I'll approve for now and we can do a follow-up (so I can finally uninstall postman 😆 )

Co-authored-by: Max Kurapov <[email protected]>
@sabineschaller sabineschaller mentioned this pull request Feb 29, 2024
6 tasks
@sabineschaller
Copy link
Member Author

Issue with outstanding discussion items:

@sabineschaller sabineschaller merged commit b462c8b into main Feb 29, 2024
22 checks passed
@sabineschaller sabineschaller deleted the s2-bruno branch February 29, 2024 10:47
@sabineschaller sabineschaller mentioned this pull request Mar 7, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: documentation Changes in the documentation package. pkg: mock-ase type: ci Changes to the CI type: documentation (archived) Improvements or additions to documentation type: localenv Local playground
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants