Add order-worker. #11
Conversation
robholland
requested review from
cretz,
tomwheeler and
paulnpdev
as code owners
There was a problem hiding this comment.
main.go would be the consistent name for this file with the other cmd
| ) | ||
|
|
||
| var rootCmd = &cobra.Command{ | ||
| Use: "order-worker", |
There was a problem hiding this comment.
No need to change anything, but can get a bit confusing calling this "order" worker that does "order" and "shipment" stuff. I think there is ambiguity on whether "order" means "order" things in code or the general name of the app/worker.
There was a problem hiding this comment.
Paul, Rob, and I discussed this. We agree with the comment, but feel that it's probably a short-term issue, as the single Worker will likely be split into more specialized Workers later (e.g., one for Orders and another for Shipments). If that doesn't happen before release, then we'll find a better name for this one.
| Use: "order-worker", | ||
| Short: "Worker for Order system", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| c, err := client.Dial(client.Options{}) |
There was a problem hiding this comment.
Doesn't have to be now, but at some point we're going to want people to be able to configure their endpoint externally (i.e. ability to run reference app against cloud)
There was a problem hiding this comment.
@cretz What would you recommend as being the most convenient way to do that? Through environment variables, for example? If so, I can certainly update the code to do that.
There was a problem hiding this comment.
We are actually exploring this as a general feature need for Temporal at temporalio/features#441. In the meantime, anything is probably ok (e.g. env vars is fine).
There was a problem hiding this comment.
I plan to submit a PR that updates it to use environment variables. I'll use the same as those already used by the Temporal CLI, for consistency.
There was a problem hiding this comment.
Looks good to me. I ran the code successfully and was able to manually process both a single shipment and an order with multiple shipments.
I do agree with Chad that the naming is slightly confusing, since this Worker handles both Order and Shipment Workflows.
I also agree that renaming the file from worker.go to main.go would make it more consistent with this example as well as other Go examples that Temporal provides (such as those in the samples-go repository). The naming of the parent directory already conveys that this is a Worker.
| Use: "order-worker", | ||
| Short: "Worker for Order system", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| c, err := client.Dial(client.Options{}) |
There was a problem hiding this comment.
@cretz What would you recommend as being the most convenient way to do that? Through environment variables, for example? If so, I can certainly update the code to do that.
| ) | ||
|
|
||
| var rootCmd = &cobra.Command{ | ||
| Use: "order-worker", |
There was a problem hiding this comment.
Paul, Rob, and I discussed this. We agree with the comment, but feel that it's probably a short-term issue, as the single Worker will likely be split into more specialized Workers later (e.g., one for Orders and another for Shipments). If that doesn't happen before release, then we'll find a better name for this one.
| Use: "order-worker", | ||
| Short: "Worker for Order system", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| c, err := client.Dial(client.Options{}) |
There was a problem hiding this comment.
I plan to submit a PR that updates it to use environment variables. I'll use the same as those already used by the Temporal CLI, for consistency.
No description provided.