Closed leggetter closed 2 weeks ago
I think this is a fair idea. It does make sense that we want to keep user on their terminal, especially when they're configuring some their development configuration anyway.
I'm curious if a better solution is to encourage better adoption of Terraform? Although I can see how it's not possible for every team.
From the 2 options you have, I think option 1 is way more viable. I feel the complexity of adding feature to the listen command may not be worth it. Can you share the TUI UX you have in mind?
What about dynamically changing if a --cli-path {path}
flag is passed to the listen command
?
This allows:
listen
is called@leggetter what if a source has multiple connections to different CLI destinations, how do we know which destination to override?
@alexluong, how does the CLI know which destination to inherit and use when it connects, and multiple CLI Destinations exist?
@leggetter it doesn't, it just sends the events to every connected destination when there's a new event.
@alexluong, so, there must be some server-side websocket handling code that decides which CLI destination the CLI should inherit?
@leggetter my understanding is that when a request comes to the source, the server will turn that request into events to relevant destinations (via connection rules logic). The websocket server will deliver each event to the CLI, which will then proxy to the local CLI path.
However, from the CLI, it proxies based on source (you can specify the connection too but it's not relevant in this discussion). Not sure if this clarifies things. I'm not entirely sure what your idea is to go about this 😅
What about dynamically changing if a
--cli-path {path}
flag is passed to thelisten command
?This allows:
- the path to be updated dynamically when
listen
is called- the interactive prompt for the path to be bypassed
@alexluong could you create a branch to experiment with this and find any problems we need to solve?
Using a flag avoids a breaking change and/or avoids the need for additional optional prams to be passed since listen
already takes, port, source, connection query.
@mkherlakian I'm going to suggest we don't change the default behaviour to prompt for a CLI path. We could change to default to /
but this feels like a breaking change.
@mkherlakian I'm going to suggest we don't change the default behaviour to prompt for a CLI path. We could change to default to / but this feels like a breaking change.
Ok. But how do you choose the connection that this applies to in this case? Without a connection, the CLI doesn't know which destination to update unless there's only one dest
Hey @leggetter, so let me elaborate the point a bit just to make sure we understand your idea a bit better. Let's say we have a connection set up like this where a source has 2 connections to 2 CLI destinations. This is how it looks when proxying:
$ hookdeck-cli git:(main) hookdeck listen 4000
? Select a source source
Dashboard
👉 Inspect and replay events: https://dashboard.hookdeck.com?team_id=tm_odIiTVdHmb4a
source Source
🔌 Event URL: https://hkdk.events/rmvvtxrgic256w
Connections
local-2 forwarding to /local/2
local-1 forwarding to /local/1
> Ready! (^C to quit)
So, from your idea, if we run $ hookdeck listen 4000 --cli-path /local/another
, how should we decide between local-1
and local-2
destinations?
I understand your intention is to make changing CLI path easier, but we need to think through these edge cases before we can proceed with the simple use case.
Besides, another case we should think about is the multi-source support that we will support soon. In that case, there are many different destinations with different path, how should we determine which path to override?
@alexluong thanks for clarifying. @mkherlakian see above. Would providing a connection name resolved then problem?
hookdeck listen 3030 source connection --cli_path /webhooks
Honestly, for newcomers who don't know about sources and connections it adds another unknown param sonim not super happy about it.
Would providing a connection name resolved then problem?
Yes, this would sort of resolve the problem. The only thing is I don't see a ton of value in this, but we can make this work.
Basically this will override the CLI path. That's great for temporary work, but if this is a long-term change, than wouldn't updating the destination be better for future usage as well as collaboration?
Honestly, for newcomers who don't know about sources and connections it adds another unknown param sonim not super happy about it.
The concern is this may add more complexity instead of reducing it. The thing is the user must set up their connection & destination first, one way or the other, before doing this. Therefore, I don't quite see the problem this solution is solving.
The only thing is I don't see a ton of value in this
Why not?
Basically this will override the CLI path. That's great for temporary work, but if this is a long-term change, than wouldn't updating the destination be better for future usage as well as collaboration?
The way I see it, we should be updating the destination if the path param is specified in the CLI args, or creating it if it doesn't exist.
The thing is the user must set up their connection & destination first, one way or the other, before doing this.
Why can't the connection and destination be created on the fly?
The concern is this may add more complexity instead of reducing it.
On the contrary - If we instruct users to just issue the one command, and everything is created for them, even if they are not familiar with source, dest, and connection constructs, they can get started immediately. It reduces friction quite a bit. Ideally we can do this with a guest accounts as well.
Why can't the connection and destination be created on the fly?
They can, but not with just cli-path
. We need at the very least the destination name as well.
It seems I'm not quite following the purpose of what we're trying to do here. Maybe @leggetter or @mkherlakian can help write a short summary to make sure we're aligned.
From this original message:
I thought this is to assist with the UX for continued usage instead of the initial adoption. But from your message @mkherlakian, it seems you're focusing more on the initial friction and user experience?
I'm also confused between 2 goals:
cli-path
of an already-created destination)So overall it's just a bit difficult what we're trying to solve for. Either of those, or both of those, are all good, but I hope we should clarify the intention so we can solve the right problem.
@alexluong @mkherlakian have a look at this https://github.com/hookdeck/hookdeck-cli/pull/90
Try:
./hookdeck-cli listen 8080 source_name --cli-path /some_path --log-level debug
(jumping on a flight so this is a quick draft PR)
Thanks for the PR @leggetter, so from what I can see, here are the key behaviors I noticed from the PR:
cli
.--cli-path
and the destination value, then we will update it on Hookdeck as well.--cli-path
config.I think we can certainly work with this if this is the behavior you'd like. Maybe some small improvements around logging to communicate what we're doing to avoid confusion, but other than that I do think this should work if the behaviors above make sense to yall.
That works, thanks @leggetter!
Let's polish and release.
It would be nice to solve the multiple destinations problem too - maybe if you pass a CLI path that matches one of the connections, you select that connection. And if it doesn't match, you create that destination?
Considering another approach - How about if we gave the option to the user to provide a connection name?
Problem is listen now accepts a source, would be difficult to distinguish between source and connection.
maybe if you pass a CLI path that matches one of the connections, you select that connection. And if it doesn't match, you create that destination?
This could work, so if you pass cli-path
, that guarantees that you'll always proxy to a single connection (unless multiple connections with the same CLI destination matching the path).
Problem is listen now accepts a source, would be difficult to distinguish between source and connection.
You can pass a source AND a connection.
$ hookdeck listen 4000 source_name connection_name
We actually already support the CLI path in place of connection_name
here too actually, maybe something to think about?
Let's polish and release.
@leggetter let me know if you'd like to polish yourself or if you'd like my help here!
@mkherlakian @alexluong here's another pass https://github.com/hookdeck/hookdeck-cli/pull/92
I've mapped out various scenarios and shown the old behavior and the new behavior.
A reasonably big functional change is to remove the interactivity. Reasoning is in the PR.
Fixed in #97
Running the following will prompt for a path for the CLI:
However, after this point, if you want to change the path, you need to go to the dashboard and edit from the UI.
Can we add support for setting the CLI path on the original request and changing the "CLI path" for a Destination from the CLI?
There's a great discussion and some good work gone into https://github.com/hookdeck/hookdeck-cli/pull/68 as an experiment for adding broader CRUD support and we ultimately decided against it (for now).
However, this feels like a common sense feature to support.
Options I can think of:
@alexluong WDYT?