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

Adds --cli-path and changes behavior to remove connection name and path terminal interactivity #92

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

leggetter
Copy link
Collaborator

The interactive prompts for the connection label/name and the CLI path are confusing unless you already understand what the Hookdeck mental model of Source -> Connection -> Destination. Why force this mental model upon developers? Instead, use some sensible defaults and allow the connection name and CLI path to be optionally passed for use when developers need or want to understand the mental model.

New defaults

  • Connection name: cli (Note: The SDK doesn't allow -> in the name or I'd have used {source} -> {destination}). We could instead use {source}-to-{destination}
  • CLI path /, which also means that the default functionality matches that of ngrok

The quickstart command will now be the following and will require no interaction within the terminal:

hookdeck listen {port} {source_name}

We could have a default for the Source name. However, I don't know what this would be, and thinking about where the event is coming from is a reasonable step towards the Hookdeck mental model.

Behavior

For Old = New behavior, ✅ indicates the behavior has not changed. ⚠️ indicates a change in behavior.

Command Connection Exists? Old = New behavior Old New
hookdeck listen {port} No ⚠️ Prompts for source, connection, and path. Creates new connection with user provided details. Prompts for source. Creates new connection with default Connection and Destination details.
hookdeck listen {port} {source} No ⚠️ Prompts for path and connection. Creates new connection with user provided details. Creates new connection with default Connection and Destination details
hookdeck listen {port} {source} Yes Runs using existing connections between {source} and all CLI Destinations. Runs using existing connections between {source} and all CLI Destinations.
hookdeck listen {port} {source} --cli-path /webhooks No N/A N/A Creates new connection. Runs with /webhooks as path all other default details.
hookdeck listen {port} {source} --cli-path /webhooks Yes N/A N/A Runs using existing connection between {source} and a CLI Destination. If existing Destination CLI path differs from /webhooks it updates the Destination. If more than one matching connection is found, asks the user to specify a connection (see Note 1).
hookdeck listen {port} {source} {connection} No ⚠️ Prompts for path. Creates new connection with user provided details. Creates new connection with /webhooks as path and {connection} as connection name.
hookdeck listen {port} {source} {connection} Yes Runs using connection that matches provided details. Runs using connection that matches provided details.
hookdeck listen {port} {source} {connection} --cli-path /webhooks No N/A N/A Creates new connection named {connection} with default Connection and Destination name but with /webhooks as path.
hookdeck listen {port} {source} {connection} --cli-path /webhooks Yes N/A N/A Runs using existing connection named {connection} between {source} and a CLI Destination. If existing Destination CLI path differs from /webhooks it updates the Destination CLI path. If more than one matching connection is found, asks the user to specify a connection (see Note 1).

Note 1

DEBUG output only present due to `--log-level debug' flag.

./hookdeck-cli listen 3000 test-inbound  --cli-path /flarb --log-level debug
[Tue, 23 Jul 2024 17:29:00 BST] DEBUG Connection exists for Source "test-inbound", Connection "", and CLI path "/flarb"
Multiple CLI destinations found. Cannot set the CLI path on multiple destinations.
Specify a single destination to update the CLI path. For example, pass a connection name:

  hookdeck listen http://localhost:3000 test-inbound connection-name --cli-path /flarb

@alexluong
Copy link
Collaborator

alexluong commented Jul 23, 2024

We could have a default for the Source name. However, I don't know what this would be

Since source name is just to identify the source but doesn't have any meaningful impact, we can potentially generate a random name (maybe "adjective noun random_number")?


Sharing this message I sent in the other PR, curious to hear if you agree with this thought process here.

#90 (comment)

CleanShot 2024-07-23 at 23 51 58

@leggetter
Copy link
Collaborator Author

We could have a default for the Source name. However, I don't know what this would be

Since source name is just to identify the source but doesn't have any meaningful impact, we can potentially generate a random name (maybe "adjective noun random_number")?

Yeah, could do. @mkherlakian - WDYT?

Sharing this message I sent in the other PR, curious to hear if you agree with this thought process here.

#90 (comment)

CleanShot 2024-07-23 at 23 51 58

It's a good question: would the user expect a side effect to take place by passing a flag? IMO it's reasonable.

@mkherlakian
Copy link
Contributor

mkherlakian commented Jul 24, 2024

@leggetter Many thanks for documenting this, very helpful!

Yeah, could do. @mkherlakian - WDYT?

Yes, or something along hte lines of source_<random_letters_numbers>

It's a good question: would the user expect a side effect to take place by passing a flag? IMO it's reasonable.

I don't think that we need to explicitly disallow the switching of the path, i.e. no need for a separate flag IMO

Runs using existing connection named {connection} between {source} and a CLI Destination. If existing Destination CLI path differs from /webhooks it updates the Destination CLI path. If more than one matching connection is found, asks the user to specify a connection (see Note 1).

I'm trying to map out scenarios for this one - Is there a potential.case where we'd end up creating 2 cli destinations (ie 2 connections) on separate runs without the user specifying a connection explicitly, which would lead to the above becoming a friction point?
i.e. if I do
hookdeck listen {port} {source} --cli-path /webhooks

and then do
hookdeck listen {port} {source} --cli-path /webhooks_new

That would update the path of the existing connection, or would it create a new one?

Last but not least, the user auth problem is somtehing we'll need to solve, meaning if we also can't do something like --api-key ... and have it work without hte CI step, it still feels janky.

@alexluong
Copy link
Collaborator

That would update the path of the existing connection, or would it create a new one?

@mkherlakian from the table that @leggetter described, it seems your workflow would result in 2 separate connections / destinations.

Last but not least, the user auth problem is somtehing we'll need to solve, meaning if we also can't do something like --api-key ... and have it work without hte CI step, it still feels janky.

Are you referring to this issue #89? Can you take a look? I think this is an issue with the API or with the design and not a CLI bug.

@mkherlakian
Copy link
Contributor

That would update the path of the existing connection, or would it create a new one?

@mkherlakian from the table that @leggetter described, it seems your workflow would result in 2 separate connections / destinations.

No, not according to the second row, I just reread, think that answers the question but I'll wait for @leggetter's confirmation
image

Last but not least, the user auth problem is somtehing we'll need to solve, meaning if we also can't do something like --api-key ... and have it work without hte CI step, it still feels janky.

Are you referring to this issue #89? Can you take a look? I think this is an issue with the API or with the design and not a CLI bug.

I'll answer on that issue.

@leggetter
Copy link
Collaborator Author

@leggetter Many thanks for documenting this, very helpful!

Yeah, could do. @mkherlakian - WDYT?

Yes, or something along hte lines of source_<random_letters_numbers>

Ok, I'll look into this.

Runs using existing connection named {connection} between {source} and a CLI Destination. If existing Destination CLI path differs from /webhooks it updates the Destination CLI path. If more than one matching connection is found, asks the user to specify a connection (see Note 1).

I'm trying to map out scenarios for this one - Is there a potential.case where we'd end up creating 2 cli destinations (ie 2 connections) on separate runs without the user specifying a connection explicitly, which would lead to the above becoming a friction point?
i.e. if I do hookdeck listen {port} {source} --cli-path /webhooks

and then do hookdeck listen {port} {source} --cli-path /webhooks_new

That would update the path of the existing connection, or would it create a new one?

It is possible to create two connections to the CLI with other commands. I discovered this with the current CLI when creating the CLI/Docker quickstart example. And that's why the cURL command in that example passes a Connection name (note that with this CLI update the cURL command is no longer required in the Docker example).

However, when the --cli-path is supplied, additional logic kicks in. The logic checks to ensure there is only one matching connection to be updated. If more than one connection is found, the user is informed to pass a {connection} to ensure one unique connection has the Destination path updated.

Last but not least, the user auth problem is somtehing we'll need to solve, meaning if we also can't do something like --api-key ... and have it work without hte CI step, it still feels janky.

The following works with the API key (as shown in the docker example):

hookdeck ci --api-key $HOOKDECK_API_KEY
hookdeck listen $PORT $SOURCE_NAME $CONNECTION_NAME --cli-path /webhook

That said, #89 does highlight unexpected behavior.

@mkherlakian
Copy link
Contributor

It is possible to create two connections to the CLI with other commands. I discovered this with the current CLI when creating the CLI/Docker quickstart example. And that's why the cURL command in that example passes a Connection name (note that with this CLI update the cURL command is no longer required in the Docker example).

However, when the --cli-path is supplied, additional logic kicks in. The logic checks to ensure there is only one matching connection to be updated. If more than one connection is found, the user is informed to pass a {connection} to ensure one unique connection has the Destination path updated.

Perfect, that's what we want

The following works with the API key (as shown in the docker example):

hookdeck ci --api-key $HOOKDECK_API_KEY
hookdeck listen $PORT $SOURCE_NAME $CONNECTION_NAME --cli-path /webhook
That said, #89 does highlight unexpected behavior.

Yes, I'm answering on the issue itself - this works because ci creates a cli user. It's janky though because the user won't see the session in their dashboard - it'll fall back eventually because we look for connected sessions and assign one, nit it's not "clean"

@leggetter leggetter marked this pull request as ready for review July 25, 2024 15:03
@leggetter leggetter merged commit 42fb1e0 into listen-multiple Jul 25, 2024
@leggetter leggetter deleted the feat/cli-path-non-interactive branch July 25, 2024 15:03
@leggetter leggetter mentioned this pull request Jul 25, 2024
@leggetter leggetter restored the feat/cli-path-non-interactive branch July 29, 2024 13:15
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