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

v0.2.0 #84

Merged
merged 34 commits into from
Jul 29, 2024
Merged

v0.2.0 #84

merged 34 commits into from
Jul 29, 2024

Conversation

alexluong
Copy link
Collaborator

@alexluong alexluong commented Jun 5, 2024

Adds support for:

Listening on multiple sources #70

$ hookdeck listen 1234 source-1,source-2
$ hookdeck listen 1234 '*'

Notes: Based on the way our CLI processes argument, when running hookdeck listen 1234 *, it will pass every files in the directory as arguments, causing a validation issue. Therefore, we need to do hookdeck listen 1234 '*' for it to work. Curious what yall think about this. It may make more sense to change it to a flag like hookdeck listen 1234 --all or hookdeck listen 1234 --all-source.

Defaults for optional listen command connection and path arguments

This removes the interactivity prompts for the connection "label" and "path".

See #92.

Ability to set the CLI Path using --cli-path flag

See #92.

@alexluong alexluong marked this pull request as ready for review June 6, 2024 09:22
@leggetter
Copy link
Collaborator

leggetter commented Jun 6, 2024

@alexluong I much prefer:

hookdeck listen 1234 --all-sources

Over

hookdeck listen 1234 '*'

I also think we should support the following:

hookdeck listen 1234 "source-1 source-2 source-3"

hookdeck listen 1234 "source-1, source-2, source-3"

@alexbouchardd
Copy link
Contributor

Can we look into the root cause as to why * itself doesn't work? Shouldn't that be something we can change?

hookdeck listen 1234 --all-sources is inconsistent with the params semantic and looses the ability to query connections since you no longer have a source param

@alexbouchardd
Copy link
Contributor

Because we use connection query and limit it to 1 connection, in the sources section we still show all sources. Should we only show sources that we're currently proxying?

👍 Make sense to only display the one that can generate events

@alexluong
Copy link
Collaborator Author

Can we look into the root cause as to why * itself doesn't work? Shouldn't that be something we can change?

Yes, this is on my TODO list. I think it has something to do with the Cobra package or how Go flags work but haven't looked further. It's pretty hard to Google tho 😅

@leggetter
Copy link
Collaborator

See https://stackoverflow.com/a/2755803/39904 for the reasoning behind the *.

User's can do \* or set some command prompt flag prior to using the CLI. We're not going to easily get the behavior we're looking for.

@alexluong
Copy link
Collaborator Author

Thanks @leggetter! How would you suggest we go about this then?

@alexluong
Copy link
Collaborator Author

alexluong commented Jun 6, 2024

@leggetter @alexbouchardd I pushed some new commits that addressed your feedback, just have the * issue left.

@alexbouchardd
Copy link
Contributor

Right * is out of the question then. I'm leaning toward '*' since it preserves the argument position over a flag. It sounds like \* would also work...

Copy link
Contributor

@alexbouchardd alexbouchardd left a comment

Choose a reason for hiding this comment

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

Approving but waiting on final updates

pkg/listen/connection.go Outdated Show resolved Hide resolved
pkg/proxy/proxy.go Show resolved Hide resolved
@alexluong
Copy link
Collaborator Author

Right * is out of the question then. I'm leaning toward '*' since it preserves the argument position over a flag. It sounds like * would also work...

The code should already work with both of these!

@leggetter please feel free to QA and merge whenever you're ready. I'm happy with where things are so we should be good to move forward here.

@leggetter
Copy link
Collaborator

leggetter commented Jul 16, 2024

@alexluong I still think this will confuse people, but we can publish and share see what feedback we have.

One last question. When I run hookdeck listen 3030 '*' I'd expect to see:

  • X
  • vonage-local
  • twilio-local
  • cli

All connected. However, I only get the first two. Why is that? I can see we have a temp limit on the sources we query when it feels like we should impose that on the number of sources we can listen to.

CleanShot 2024-07-16 at 16 26 52

@alexluong
Copy link
Collaborator Author

@leggetter That's fair. What do you think is a better DX though? We can potentially have a more "elaborate" mode where we showcase the process:

  • query 10 sources, here are the results...
  • query connections, here are the results...
  • of those connections, here are the connections pointing to a CLI destination
  • get sources linked to those connections
  • ... the rest that you can see

All connected. However, I only get the first two. Why is that?

The current logic is we query 10 sources with Hookdeck's default sort & filter logic. Most likely that request yielded you the 10 sources, 8 of which without a CLI destination. Unfortunately, we can't query something like "give me 10 sources with connections to CLI destination", I don't think.

@leggetter
Copy link
Collaborator

leggetter commented Jul 16, 2024

@alexluong, is there any reason we can't query 100 sources but restrict the number of Sources we connect to the first 10 we find?

Is the problem here that we'll potentially be making 101 requests? 1 to request the sources and potentially 100 to filter through the connections with CLI destinations.

@alexluong
Copy link
Collaborator Author

@leggetter yes, that is essentially the case. The only difference is we'll make 101 requests, 100 request to query for sources (because we can only send 1 name) and 1 request to query for connections.

@leggetter leggetter changed the title feat: Listen command can proxy multiple sources v0.2.0 Jul 25, 2024
@leggetter
Copy link
Collaborator

@alexluong - would you mind giving the updates on the merge of #92 into this a review and update?

@alexluong
Copy link
Collaborator Author

@leggetter yes, I'll take a deeper look tomorrow. Is there anything you'd like me to update?

@leggetter
Copy link
Collaborator

@leggetter yes, I'll take a deeper look tomorrow. Is there anything you'd like me to update?

Nothing functionally.

So:

  1. A once-over of the code. I'm not a Go developer so I may well have added code that smells weird 😄
  2. Some general Q&A would also be great

@alexluong
Copy link
Collaborator Author

@leggetter Got it, so moreso code polishing & making sure there's no issue and just keep the functionality the same as laid out in #92.

@leggetter
Copy link
Collaborator

@leggetter Got it, so moreso code polishing & making sure there's no issue and just keep the functionality the same as laid out in #92.

Yes, 👍 Thank you!

pkg/listen/listen.go Outdated Show resolved Hide resolved
pkg/listen/listen.go Outdated Show resolved Hide resolved
@leggetter leggetter merged commit c51251b into main Jul 29, 2024
4 checks passed
@leggetter leggetter deleted the listen-multiple branch July 29, 2024 13:58
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.

3 participants