-
Notifications
You must be signed in to change notification settings - Fork 226
Include Expected Outputs to Hello World Sample #439
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
Conversation
|
|
04557e0 to
b5a30fb
Compare
yuandrew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement! Some cleaning up to do, but overall agree it's helpful for samples to give expected output so users know it was run successfully.
helloworld/README.md
Outdated
| 1) Run a [Temporal server](https://github.com/temporalio/samples-go/tree/main/#how-to-use). (If you are going to run locally, you will want to open and run in another terminal as the command is a blocking process that runs until it receives a SIGINT (Ctrl + C) command.) | ||
|
|
||
|
|
||
| You should see initialization logs, the last will will indicate `workflow successfully started`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log seems unrelated to the sample, it's running a workflow named temporal-sys-history-scanner-workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, those are validation instructions for the user to know that the server is up and running successfully. Agree it is more related to the server up and running, but I think that for a Hello World sample it is okay to have extra validation info to save the user jumping back and forth between this doc and the Temporal Server doc. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line isn't related to the server running though, but instead that a workflow has run. So far in the instructions a user hasn't touched a workflow yet. Server output is usually something like
➜ ~ temporal server start-dev
CLI 1.5.1 (Server 1.29.1, UI 2.42.1)
Server: localhost:7233
UI: http://localhost:8233
Metrics: http://localhost:53085/metrics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(referring to the workflow successfully started and the full log line below, just in case that wasn't clear)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. That wasn't the correct server output.
I actually was using this sample in conjunction with self building the temporal server (https://github.com/temporalio/temporal/blob/main/CONTRIBUTING.md) so I had different server output (and I think I had pasted the the wrong log line even for that...). That said, this sample should assume that the server is using the CLI to build and run the server, so it should have the output you suggested.
helloworld/README.md
Outdated
| {"level":"info","ts":"2025-12-22T14:53:19.673-0500","msg":"workflow successfully started","service":"worker","wf-type":"temporal-sys-history-scanner-workflow","logging-call-at":"/repo/temporal/service/worker/scanner/scanner.go:273"} | ||
| ``` | ||
|
|
||
| 2) Create a new namespace named `default` using this command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default namespace works without manually creating it. I'm not sure it's worth mentioning that non-default namespaces need to be created in this sample. It feels out of the scope of this sample, and we already have this info in docs https://docs.temporal.io/develop/go/namespaces#register-namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, but https://github.com/temporalio/temporal/blob/main/CONTRIBUTING.md now also directs users to try out the Hello World samples, and when starting the server using those instructions it does not create the default namespace.
I believe the best way to balance these two concerns would be to optimize the guide for the server being started via CLI (don't include the create namespace as a step) but instead include a debugging tip at the bottom for what to do if the Worker won't start because of the missing namespace.
yuandrew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement! LGTM, one small formatting comment to consider
|
|
||
| ## Troubleshooting | ||
|
|
||
| > [!NOTE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure having this all in a NOTE block is necessary, it's already in its own Troubleshooting block.
But great detail to add! TBH I wasn't aware of this fact until now haha
Co-authored-by: Andrew Yuan <andrew.yuan@temporal.io>
Co-authored-by: Andrew Yuan <andrew.yuan@temporal.io>
d40deec to
84d609c
Compare
What was changed
Documentation Change only. Update the README.md to 1) Include expected outputs, 2)Add instruction to create the default namespace if it does not exist, 3) Highlight that the run workers command is a blocking command (so run in another window),
Why?
Because sample instructions should be unambiguous about what success looks like and have helpful tips to aid in running. It should should aspire to the don't make me think principle.