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

Fixes workflow event states unmarshalling methods #65

Merged
merged 5 commits into from
Aug 29, 2022
Merged

Fixes workflow event states unmarshalling methods #65

merged 5 commits into from
Aug 29, 2022

Conversation

andresmijares
Copy link
Contributor

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:
Actually, there's a small bug preventing to have Event Switch State and Event Data State on the same definition, this creates a small case where the other of the states only works if they are placed certain way, it's not deterministic.

We are using the sdk but we had to fork it in order to make it work correctly, I think, others will benefit from it.

How to reproduce it.

Given this definition:

{
		"id": "testWorkflow",
		"name": "Validation Workflow Test",
		"version": "1.0",
		"specVersion": "0.7",
		"start": "Sample",
		"states": [
                       {
				"name": "Sample",
				"type": "switch",
				"dataConditions": [
				  {
					"condition": "${  .total > 10 }",
					"transition": "Sample2"
				  }
				]
		      },
                       {
				"name": "Sample2",
				"type": "switch",
				"eventConditions": [
					{						
                                                  "eventRef": "eventOne",
						"transition": "HandleEventFinishOne"
				  	},
					{
						"eventRef": "eventFinishTwo",
						"transition": "HandleEventFinishTwo"
				  	}
				]
			}
                 ]
}

This line it's causing that when trying to unmashalling Sample2, mapState is not cleared for the next loop iteration, so map[dataConditions] is not nil. This cause this line to be trigger wrongly, when there are 2 switch events and a Switch Event Database is place before the Switch Event.

Clearing mapState before the next iteration of the loop should solve the issue.

Please, let me know, if anything else is needed.

Special notes for reviewers:

Additional information (if needed):

@andresmijares andresmijares changed the title clear mapState after next loop iteration Fixes workflow event states unmarshalling methods Aug 25, 2022
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for this change. Can you please add a unit test to validate the unmarshaling? Many thanks!

@andresmijares
Copy link
Contributor Author

@ricardozanini should be good now, I added a small case with both states and making sure it compiles which is the small issue I'm solving, you can quickly confirm the issue changing the code back to the original way and run the test. Let me know if something else is required ^^. Thank u!

@ricardozanini
Copy link
Member

@andresmijares let me know if you need a new patch release.

@andresmijares
Copy link
Contributor Author

@ricardozanini after the merge, that would be great! i've been using a fork of the project only cause this :)

@ricardozanini
Copy link
Member

@ricardozanini after the merge, that would be great! i've been using a fork of the project only cause this :)

Actually, I'll upgrade the Go version and try to fix the CI. Something is not quite right. See #63

@ricardozanini
Copy link
Member

@andresmijares, I've fixed all these CI bugs and dependencies problems here: #66

Can you please take a look and try that PR locally to see if it's fine on your side so I can merge it? We then rebase this and do a release.

@andresmijares
Copy link
Contributor Author

andresmijares commented Aug 29, 2022

@ricardozanini i just sourced this branch on my project and ran without problem (without considering the issues fixed in this PR)!

go version go1.18.3 darwin/amd64

Just in case

@ricardozanini
Copy link
Member

@andresmijares can you rebase now that I've merged #63?

@andresmijares
Copy link
Contributor Author

@ricardozanini done! thank you!

@andresmijares
Copy link
Contributor Author

@ricardozanini last question, not related to the PR, is there any slack channel for ppl sharing ideas about this project?

@ricardozanini
Copy link
Member

ricardozanini commented Aug 29, 2022

@ricardozanini last question, not related to the PR, is there any slack channel for ppl sharing ideas about this project?

Sure thing: https://cloud-native.slack.com/

Go to serverless-workflow-sdk channel and ping me there. I'm online during working hours.

@ricardozanini ricardozanini merged commit c6596fd into serverlessworkflow:main Aug 29, 2022
@andresmijares
Copy link
Contributor Author

andresmijares commented Aug 29, 2022

@ricardozanini i need an invitation, could u send me one pls? email is github user, gmail, thanks!!

@ricardozanini
Copy link
Member

@andresmijares, many thanks for this contribution. I'm releasing a new patch version this afternoon.

@ricardozanini
Copy link
Member

@andresmijares, the slack channel is open. I tried to add you to the channel, but it has been disabled. IDK why. You should be able to join, nevertheless.

@andresmijares
Copy link
Contributor Author

@ricardozanini i think it's disabled, i tried joining and it says i dont have an account and i need one :(

@ricardozanini
Copy link
Member

You just have to create a free account, I believe.

@andresmijares
Copy link
Contributor Author

it seems it is set as invited only, not public joining option

@ricardozanini
Copy link
Member

@andresmijares try this https://communityinviter.com/apps/cloud-native/cncf

@andresmijares
Copy link
Contributor Author

thank you!

@ricardozanini
Copy link
Member

Worked?

@andresmijares
Copy link
Contributor Author

yes sir!

@ricardozanini
Copy link
Member

Great, I'll update our docs.

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.

None yet

2 participants