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

Updates for temporal CLI over tctl and temporal dev-server #131

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

cici
Copy link
Contributor

@cici cici commented Jul 23, 2024

What was changed

I updated the README to use the temporal CLI instead of tctl and updated codec_server.py to default to port 8233 for the local web UI since that is the default port for temporal server start-dev

Why?

Checklist

  1. Closes

  2. How was this tested:

Tested on my local machine

  1. Any docs updates needed?

Done

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Thanks! One minor note about the env var.

Comment on lines 7 to 11
Ensure you have an environment variable set to the Namespace your Workflows are in:


export TEMPORAL_NAMESPACE=<Namespace Name>

Copy link
Member

Choose a reason for hiding this comment

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

This variable is not used in this sample, the namespace is fixed to default (we expect to come later and make all samples run somewhere configurable)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that in because my TEMPORAL_NAMESPACE was set to my Cloud Namespace and when I ran the starter, I got an error that the Namespace didn't exist, so I reset it to "default" and then it ran fine.

Copy link
Member

@cretz cretz Jul 23, 2024

Choose a reason for hiding this comment

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

The starter in the encryption sample doesn't use env vars. Do you maybe mean the temporal CLI which does? There may be other env vars the CLI uses that you would have to make sure are unset if you're concerned about your existing environment variable leftovers polluting the CLI (though I can't think of any for dev server). Also, would encourage unset TEMPORAL_NAMESPACE instead of export TEMPORAL_NAMESPACE=<Namespace Name> and "Namespace your Workflows are in". This sample is hardcoded to work with default.

Copy link
Contributor Author

@cici cici Jul 24, 2024

Choose a reason for hiding this comment

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

I wasn't using the CLI to start the Workflow. I do see where it is using default, so I will have to debug what I was seeing. I will remove that statement.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Looks great! You'll need to merge main. Also let me know if it doesn't let you click the merge button and I'll click it.

@cretz cretz merged commit a0b2cdf into main Jul 24, 2024
9 checks passed
@cretz cretz deleted the updates-for-cli branch July 24, 2024 17:14
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.

2 participants