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

Grant-onboarding #12

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

Grant-onboarding #12

wants to merge 8 commits into from

Conversation

GSmithApps
Copy link

Some things noted by fresh eyes

One is important -- the some import statements in the practice code were importing from the solution

Comment on lines -19 to +22
1. Edit the `workflow.go` file. You will use the `workflow.SetQueryHandler()` to add Query handling to your Workflow. This can be done anywhere inside of the `Workflow()` function. In this case, you'll add a query to return the Workflow's `currentState`, so it makes sense to initialize the `currentState` variable as you go along. Note that this is the same Workflow from the previous exercise, so it is designed to wait for a Signal after starting.
1. Edit the `workflow.go` file.

You will use the `workflow.SetQueryHandler()` to add Query handling to your Workflow. This can be done anywhere inside of the `Workflow()` function. In this case, you'll add a query to return the Workflow's `currentState`, which you'll modify as the Workflow progresses. Note that this is the same Workflow from the previous exercise, so it is designed to wait for a Signal after starting.

Copy link
Author

Choose a reason for hiding this comment

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

Tom suggested to me one time to separate prose from instruction/commands. I tried to do that here -- totally up to you on whether or not to keep it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just adding a linebreak is a pretty reasonable way of doing that imo! I'd previously rejected suggestions along these lines that distorted the number of actual steps there were, but this works fine. Thanks!

Comment on lines -33 to +39
4. Finally, update `currentState` again after that block of code, to something like "waiting for signal". This is the state that the Workflow will be waiting at when we query it.
5. Save the file.
4. Update `currentState` again after that block of code, to something like "waiting for signal". This is the state that the Workflow will be waiting at when we query it.
5. Finally, update `currentState` to something like `"Workflow Complete"`
after the Activity has completed successfully near where the Workflow completion is logged.
6. Save the file.
Copy link
Author

Choose a reason for hiding this comment

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

I added this note to change to add a statement for workflow complete. It used to be in part E, but I just moved it here. I think it's easier to have it here because I think if it's in E, the user needs to restart workers etc

Comment on lines -44 to +49
response, err := c.QueryWorkflow(context.Background(), "queries", "", "current_state")
response, err := c.QueryWorkflow(context.Background(), "queries-workflow-id", "", "current_state")
Copy link
Author

Choose a reason for hiding this comment

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

not necessary, but it had the workflow id and the task queue both as "queries", so I just changed them to disambiguate

Comment on lines -99 to +106
Now you can send a Signal to your Workflow as in the previous exercise so it completes successfully. In the terminal you sent your Query from, run `go run signalclient/main.go`.
Now you can send a Signal to your Workflow as in the previous exercise so it completes successfully.

1. In the terminal you sent your Query from, run `go run signalclient/main.go`.
Copy link
Author

Choose a reason for hiding this comment

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

separating prose from instructions. up to you 👍


## Part E: Querying a Completed Workflow

Finally, you can demonstrate querying completed Workflows. Update the `currentState` variable in `workflow.go` once more just before the Workflow returns, so that you can demonstrate querying a completed workflow. Then, re-run the workflow, and query it from the command line again:
Copy link
Author

Choose a reason for hiding this comment

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

moved to part A

queries "interacting/exercises/querying-workflows/solution"
queries "interacting/exercises/querying-workflows/practice"
Copy link
Author

Choose a reason for hiding this comment

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

significant change -- the practice code was importing from the solution, so the user's code would be working even if they didn't solve the problem yet. Lol I had this happen to me and i was pretty confused

Comment on lines -17 to +23
1. This exercise contains one Client that runs two different Workflows
— `PizzaWorkflow` and `FulfillOrderWorkflow`. Both Workflows are defined in
`workflow.go`. `PizzaWorkflow` is designed not to complete its final activity
— `SendBill` — until it receives a Signal from `FulfillOrderWorkflow`. You'll
start by defining that Signal. Edit `workflow.go`. Near the top of the file,
This exercise contains one Client that runs two different Workflows
— `PizzaWorkflow` and `FulfillOrderWorkflow`. Both Workflows are defined in
`workflow.go`. `PizzaWorkflow` is designed not to complete its final activity
— `SendBill` — until it receives a Signal from `FulfillOrderWorkflow`. You'll
start by defining that Signal.

1. Edit `workflow.go`. Near the top of the file,
Copy link
Author

@GSmithApps GSmithApps Jan 13, 2025

Choose a reason for hiding this comment

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

prose from instruction

Signal,`fulfill-order-signal`. For `SignalExternalWorkflow` calls to block
and return properly in Go, you also need to append `.Get(ctx,
[return-value-pointer])` to a `SignalExternalWorkflow` call, though
`[return-value-pointer]` can be `nil` here.
@@@@@ the course content doesn't say anything about this `.Get`. @@@@
Copy link
Author

@GSmithApps GSmithApps Jan 13, 2025

Choose a reason for hiding this comment

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

I didn't see this in the course content. I'm not sure I know what's going on here, but maybe that's okay and if this is good, then we can just keep it

related to these two: a and b

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, is this just about appending .Get() to a call so it actually blocks and returns? That's definitely worth clarifying in the course content if so.

Comment on lines -14 to -16
// TODO Part A: create a `var` named `signal` that is an instance of
// `FulfillOrderSignal` with `Fulfilled: true`. This is the Signal that
// `FulfillOrderWorkflow` will send to `PizzaWorkflow`.
Copy link
Author

Choose a reason for hiding this comment

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

we were using the signal variable as both the sending variable and the receiving variable, which I felt a little confused by. So I just made two variables -- a sent one and a received one -- both of which are in just function scopes instead of at the top

@axfelix
Copy link
Collaborator

axfelix commented Jan 13, 2025

Thank you, I love everything about Go except the way it handles local imports, which is insane to me

@axfelix
Copy link
Collaborator

axfelix commented Jan 13, 2025

And @GSmithApps don't forget to mark this as Ready for Review so I can merge your suggestions directly

@GSmithApps GSmithApps marked this pull request as ready for review January 13, 2025 19:34
@GSmithApps GSmithApps changed the title Grant-onboarding Draft: Grant-onboarding Jan 13, 2025
@GSmithApps GSmithApps marked this pull request as draft January 13, 2025 19:39
@GSmithApps GSmithApps changed the title Draft: Grant-onboarding Grant-onboarding Jan 13, 2025
@GSmithApps
Copy link
Author

GSmithApps commented Jan 13, 2025

Thanks so much for looking!! I have one change above ☝️ that isn't ready -- if you get a second to look, that would be awesome

@axfelix axfelix marked this pull request as ready for review January 13, 2025 22:38
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