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

only serialize failure details if they exist #707

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

Conversation

akshaya-a
Copy link

Description

Please explain the changes you've made

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #691

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@akshaya-a akshaya-a requested review from a team as code owners May 4, 2024 04:24
Copy link

codecov bot commented May 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.29%. Comparing base (fc0e9d1) to head (1faf3cc).
Report is 23 commits behind head on main.

Current head 1faf3cc differs from pull request most recent head c5323f1

Please upload reports for the commit c5323f1 to get more accurate results.

Files Patch % Lines
...r-ext-workflow/dapr/ext/workflow/workflow_state.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
- Coverage   86.37%   86.29%   -0.08%     
==========================================
  Files          79       82       +3     
  Lines        4094     4196     +102     
==========================================
+ Hits         3536     3621      +85     
- Misses        558      575      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elena-kolevska
Copy link
Contributor

Hey @akshaya-a , thanks for your PR. Could you please sign your commit? Here's how to do it:

Rebase the branch

If you have a local git environment and meet the criteria below, one option is to rebase the branch and add your Signed-off-by lines in the new commits. Please note that if others have already begun work based upon the commits in this branch, this solution will rewrite history and may cause serious issues for collaborators (described in the git documentation under "The Perils of Rebasing").

You should only do this if:

You are the only author of the commits in this branch
You are absolutely certain nobody else is doing any work based upon this branch
There are no empty commits in the branch (for example, a DCO Remediation Commit which was added using --allow-empty)

To add your Signed-off-by line to every commit in this branch:

Ensure you have a local copy of your branch by [checking out the pull request locally via command line](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally).
In your local branch, run: git rebase HEAD~1 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin ak/guard-workflow-state-failure-details

@elena-kolevska
Copy link
Contributor

I was able to reproduce this locally and test the fix. Looks good. I would love an added test for it, though.

@berndverst
Copy link
Member

@akshaya-a please do the following or this cannot be merged.

To add your Signed-off-by line to every commit in this branch:

Ensure you have a local copy of your branch by checking out the pull request locally via command line.
In your local branch, run: git rebase HEAD~1 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin ak/guard-workflow-state-failure-details

@akshaya-a
Copy link
Author

@elena-kolevska @berndverst sorry for the delay, was focused on some stuff for //build

yeah the only reason I didn't bother signing yet was I assumed we'd want a test covering the failure but in my project I worked around it by just reaching in to the .__obj for debugging purposes, reducing the immediate priority

i can probably get to this next week?

@akshaya-a akshaya-a force-pushed the ak/guard-workflow-state-failure-details branch from 1faf3cc to d2fe73e Compare May 18, 2024 20:15
akshaya-a and others added 2 commits May 18, 2024 20:16
Signed-off-by: Akshaya Annavajhala <akshaya-a@users.noreply.github.com>
* Pin workflow SDK 0.4.1 in example

Signed-off-by: Bernd Verst <github@bernd.dev>

* fix workflow dev version

Signed-off-by: Bernd Verst <github@bernd.dev>

* fix workflow example dependency

Signed-off-by: Bernd Verst <github@bernd.dev>

---------

Signed-off-by: Bernd Verst <github@bernd.dev>
Signed-off-by: Akshaya Annavajhala <akshaya-a@users.noreply.github.com>
@akshaya-a akshaya-a force-pushed the ak/guard-workflow-state-failure-details branch from d2fe73e to c5323f1 Compare May 18, 2024 20:16
@akshaya-a
Copy link
Author

ah didn't realize i already had a codespace for this repo, no need to be lazy - signed in case you want to merge, but happy to add a test later

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.

[WORKFLOW SDK BUG] None Check is missing in the WorkflowState to_json
3 participants