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

Go Feature Guide: Editing #2904

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

jsundai
Copy link
Contributor

@jsundai jsundai commented Jun 6, 2024

What does this PR do?

Aligning the feature guides (i.e - format, headings, etc)

Notes to reviewers

@jsundai jsundai marked this pull request as ready for review June 21, 2024 22:55
@jsundai jsundai requested a review from a team as a code owner June 21, 2024 22:55
@@ -370,7 +232,9 @@ func YourWorkflowDefinition(ctx workflow.Context, param YourWorkflowParam) (*You
}
```

### How to customize Workflow Type in Go {#customize-workflow-type}
### Customize Workflow Type in Go {#customize-workflow-type}
Copy link
Contributor

Choose a reason for hiding this comment

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

You replaced "in Go" with "using the Go SDK" above so maybe you can replace this with 'using the Go SDK' as well

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to Angela, who got there before me.

Also maybe Types instead of Type

@@ -437,7 +303,9 @@ The Temporal Go SDK has APIs to handle equivalent Go constructs:
- `workflow.Context` This is a replacement for `context.Context`.
See [Tracing](/develop/go/observability#tracing-and-context-propogation) for more information about context propagation.

## How to develop an Activity Definition in Go {#activity-definition}
## Develop an Activity Definition in Go {#activity-definition}
Copy link
Contributor

Choose a reason for hiding this comment

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

You replaced "in Go" with "using the Go SDK" above so maybe you can replace this with 'using the Go SDK' as well

@@ -567,7 +439,9 @@ func (a *YourActivityObject) YourActivityDefinition(ctx context.Context, param Y
}
```

### How to customize Activity Type in Go {#customize-activity-type}
### Customize Activity Type in Go {#customize-activity-type}
Copy link
Contributor

Choose a reason for hiding this comment

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

You replaced "in Go" with "using the Go SDK" above so maybe you can replace this with 'using the Go SDK' as well

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 also Type/Types

@@ -681,6 +559,8 @@ if err != nil {

### Go ActivityOptions reference {#activity-options-reference}

**How to use Go ActivityOptions reference using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**How to use Go ActivityOptions reference using the Go SDK**
**How to configure `ActivityOptions` using the Go SDK**

### How to set WorkerOptions in Go {#workeroptions}
### Set WorkerOptions in Go {#workeroptions}

**How to set WorkerOptions using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**How to set WorkerOptions using the Go SDK**
**How to set 1WorkerOptions` using the Go SDK**

Same for line 894

Copy link
Contributor

Choose a reason for hiding this comment

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

Except without the number 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Whooops :)

### How to set RegisterWorkflowOptions in Go {#registerworkflowoptions}
### Set RegisterWorkflowOptions in Go {#registerworkflowoptions}

**How to set RegisterWorkflowOptions using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**How to set RegisterWorkflowOptions using the Go SDK**
**How to set `RegisterWorkflowOptions` using the Go SDK**

Same for line 1433

@@ -24,9 +24,10 @@ func SimpleWorkflow(ctx workflow.Context, value string) error {

To check whether a Workflow Execution was spawned as a result of Continue-As-New, you can check if `workflow.GetInfo(ctx).ContinuedExecutionRunID` is not empty (i.e. `""`).

**Notes**
:::note
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm leaving this as a non-blocking comment as it was mostly a Patrick-driven thing as far as I understand but somewhere in our various style rules there's a rule that says we always put a title in.

New regime, we can figure it out. I don't have dog in this fight, just reporting in. Maybe check with Brian?

@@ -67,270 +71,3 @@ For more information, see [Metrics](/develop/go/observability#metrics) for setti

Debug Server performance with [Cloud metrics](/cloud/metrics/) or [self-hosted Server metrics](/self-hosted-guide/production-checklist#scaling-and-metrics).

## How to test Workflow Definitions in Go {#testing-and-debugging}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a HUGE HUGE removal of content. Was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting this here: Any removals are likely because they are being moved to a new spot to align with the migration and IA rework of the dev guides. The .NET one was a manual migration since the rest weren't, we need to make sure they are still manually moved. ✔️


This section describes how to install the [Temporal CLI](/cli) and run a development Temporal Service.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is HUGE removal of material. Just making sure it's intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't looked back at this but any removals are likely because they are being moved to a new spot to align with the migration. The .NET one was a manual migration since the rest weren't, need to make sure they are still manually moved. ✔️

@@ -86,161 +86,19 @@ tags:
description: The Foundations section of the Temporal Developer's guide introduces essential concepts for building and running a Temporal Application, outlining steps to start Workflow and Activity Execution, running development Workers, and installing the Temporal CLI.
---

The Foundations section of the Temporal Developer's guide covers the minimum set of concepts and implementation details needed to build and run a [Temporal Application](/temporal#temporal-application)—that is, all the relevant steps to start a [Workflow Execution](#develop-workflows) that executes an [Activity](#activity-definition).
This page shows how to do the following:

In this section you can find the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need both 89 and 91?

@@ -882,7 +762,9 @@ if err != nil {
}
```

### How to get the results of an Activity Execution {#get-activity-results}
### Get the results of an Activity Execution {#get-activity-results}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider 'retrieve' vs 'get'

Copy link
Contributor

Choose a reason for hiding this comment

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

Also 'Activity Execution results' is shorter and more direct than 'of an'

### How to get the results of an Activity Execution {#get-activity-results}
### Get the results of an Activity Execution {#get-activity-results}

**How to get the results of an Activity Execution using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

@@ -1009,7 +891,9 @@ func main() {
// ...
```

### How to set WorkerOptions in Go {#workeroptions}
### Set WorkerOptions in Go {#workeroptions}
Copy link
Contributor

@fairlydurable fairlydurable Aug 1, 2024

Choose a reason for hiding this comment

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

Backtick? Or space the words 'Worker options'

Also the Go SDK.

I'm going to stop mentioning the Go SDK now.

### How to set RegisterActivityOptions in Go {#registeractivityoptions}
### Set RegisterActivityOptions in Go {#registeractivityoptions}

**How to set RegisterActivityOptions using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

Backtick the code face or reword

@@ -54,7 +54,7 @@ if err != nil {

### Workflow Retry Policy {#workflow-retries}

**How to set a Workflow Retry Polciy using the Go SDK.**
**How to set a Workflow Retry Policy using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice x 2!

@@ -122,7 +122,7 @@ if err != nil {

### Set a custom Activity Retry Policy {#activity-retries}
Copy link
Contributor

Choose a reason for hiding this comment

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

I looked it up. P is upper-case, so no complaints.

@@ -40,11 +40,13 @@ The [Testing suite feature guide](/develop/go/testing-suite) shows how to set up

- [Test frameworks](/develop/go/testing-suite#test-frameworks)
- [Test setup](/develop/go/testing-suite#test-setup)
- [Test Workflow Definitions in Go](/develop/go/testing-suite#test-workflow-definitions)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Go SDK? (oops, I promised to stop, but...)

- [How to develop with Updates](/develop/go/message-passing#updates)
- [Signals](/develop/go/message-passing#signals)
- [Queries](/develop/go/message-passing#queries)
- [Updates](/develop/go/message-passing#updates)
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate that these are single words. However if they align with other SDK coverage, there's nothing to do about it.

@@ -88,7 +90,6 @@ The [Debugging feature guide](/develop/go/debugging) covers the various ways to

- [How to debug in a development environment](/develop/go/debugging#debug-in-a-development-environment)
- [How to debug in a production environment](/develop/go/debugging#debug-in-a-production-environment)
- [How to test Workflow Definitions in Go](/develop/go/debugging#testing-and-debugging)
Copy link
Contributor

Choose a reason for hiding this comment

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

using the Go SDK

@@ -137,7 +138,7 @@ The [Side Effects feature guide](/develop/go/side-effects) covers how to use Sid

- [Side Effects](/develop/go/side-effects)

## Manage Namespaces
## Namespaces
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the original

@@ -169,7 +169,7 @@ func YourWorkflowDefinition(ctx workflow.Context, param YourWorkflowParam) error

### Signal-With-Start {#signal-with-start}

**How to use Signal-With-Start using the Go SDK.**
**How to use Signal-With-Start using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

Codeface.

I have a meeting. I have to run. Sorry. Stopping here.

@@ -2,7 +2,7 @@
id: temporal-clients
title: Temporal Client - Go SDK feature guide
sidebar_label: Temporal Client
description: Learn how to connect to Temporal Service or Cloud, start Workflow Executions, manage Workflow options, and retrieve Workflow results using the Go SDK. Follow detailed steps and code examples to effectively utilize Temporal’s capabilities.
description: Learn how to connect to Temporal Service or Cloud, start Workflow Executions, manage Workflow options, and retrieve Workflow results using the Go SDK Follow detailed steps and code examples to effectively utilize Temporal’s capabilities.
Copy link
Contributor

Choose a reason for hiding this comment

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

?? Not sure if this was removed intentionally

@@ -55,6 +55,8 @@ This framework is suited for implementing unit tests as well as functional tests

## Test setup

**How to setup tests using the Go SDK**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**How to setup tests using the Go SDK**
**How to set up tests using the Go SDK**

I think "set up" is 2 words when used like this, but check me on it

and any subsequent parameters contain values for custom input parameters declared by the Workflow
function.

> Note that unless the Activity invocations are mocked or Activity implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the best way for us to do admonitions here?


After executing the Workflow in the above example, we assert that the Workflow ran through completion
via the call to `s.env.IsWorkflowComplete()`. We also assert that no errors were returned by asserting
on the return value of `s.env.GetWorkflowError()`. If our Workflow returned a value, we could have
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
on the return value of `s.env.GetWorkflowError()`. If our Workflow returned a value, we could have
the return value of `s.env.GetWorkflowError()`. If our Workflow returned a value, we could have


#### Activity mocking and overriding

When running unit tests on Workflows, we want to test the Workflow logic in isolation. Additionally,
Copy link
Contributor

Choose a reason for hiding this comment

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

"We want to" is rarely the best framing, imo... if it's not accurate to say "we need to" instead, I'd rewrite this a bit to clarify the benefits of doing it.

With the mock set up we can now execute the Workflow via the `s.env.ExecuteWorkflow(...)` method and
assert that the Workflow completed successfully and returned the expected error.

Simply mocking the execution to return a desired value or error is a pretty powerful mechanism to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Simply mocking the execution to return a desired value or error is a pretty powerful mechanism to
Mocking the execution to return a desired value or error is a pretty powerful mechanism to

@axfelix
Copy link
Contributor

axfelix commented Aug 5, 2024

Added just a few comments -- I think this is all the feedback I have on all the content that wasn't already reviewed above!

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.

4 participants