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

Error on Registering Activity with Workflow Context #1093

Conversation

shannonjmtan
Copy link
Contributor

What was changed

Check for workflow context usage within activity arguments as this is usually developer error (accidental and misunderstanding). Note that this would likely fail later on anyways when executing the function.

Why?

Prevent accidental errors.

Checklist

  1. Closes Warn on calling an ExecuteActivity on a function that takes a workflow.Context #1061

  2. How was this tested: Added unit tests for new behavior, existing tests should verify current behavior.

  3. Any docs updates needed? No as this is not stated as an allowed behavior (nor will it even work).

@shannonjmtan shannonjmtan requested a review from a team as a code owner April 20, 2023 00:46
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2023

CLA assistant check
All committers have signed the CLA.

@shannonjmtan
Copy link
Contributor Author

The original issue said to warn, but I figured this was equivalent as there was a flag to bypass failing functions. I've also not worked that closely with reflection so I'm open to alterations.

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.

LGTM. Will let @Quinn-With-Two-Ns look too.

@Quinn-With-Two-Ns
Copy link
Contributor

LGTM, just need to fix the unit test

@Quinn-With-Two-Ns Quinn-With-Two-Ns force-pushed the shannontan/checkActivityContext branch 2 times, most recently from e3ce0a4 to fb1142b Compare April 24, 2023 15:09
@shannonjmtan
Copy link
Contributor Author

Looking at the error, I think this is unrelated to my PR? Let me know if it I'm misreading.

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit a1f826a into temporalio:master Apr 24, 2023
@Quinn-With-Two-Ns
Copy link
Contributor

@shannonjmtan yep unrelated

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.

Warn on calling an ExecuteActivity on a function that takes a workflow.Context
4 participants