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.

Sign up for GitHub

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

Jump to bottom

fix nill state timeouts and enhance v08 test #104

Merged
merged 6 commits into from
Oct 20, 2022
Merged

fix nill state timeouts and enhance v08 test #104

merged 6 commits into from
Oct 20, 2022

Conversation

spolti
Copy link
Member

@spolti spolti commented Oct 10, 2022

Signed-off-by: spolti filippespolti@gmail.com

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:

Special notes for reviewers:

Additional information (if needed):

sonatype-lift[bot]
sonatype-lift bot reviewed Oct 10, 2022
View reviewed changes
model/util.go Outdated Show resolved Hide resolved
@spolti
fix nill state timeouts and enhance v08 test
dd8e5fc 
Signed-off-by: spolti <filippespolti@gmail.com>
@spolti spolti mentioned this pull request Oct 10, 2022
Closed
ricardozanini
ricardozanini reviewed Oct 10, 2022
View reviewed changes
parser/parser_test.go Outdated Show resolved Hide resolved
parser/parser_test.go Show resolved Hide resolved
@spolti spolti mentioned this pull request Oct 11, 2022
Merged
@spolti
review update
ec57196 
Signed-off-by: spolti <filippespolti@gmail.com>
ricardozanini
ricardozanini reviewed Oct 11, 2022
View reviewed changes
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.

LGTM! Many thanks! @lsytj0413 wanna take a look?

lsytj0413
lsytj0413 reviewed Oct 12, 2022
View reviewed changes
model/delay_state.go Outdated Show resolved Hide resolved
model/event.go Outdated Show resolved Hide resolved
model/event_state.go Outdated Show resolved Hide resolved
model/event_state.go Outdated Show resolved Hide resolved
parser/testdata/workflows/greetings-v08-spec.sw.yaml Outdated Show resolved Hide resolved
@lsytj0413
Copy link
Collaborator

lsytj0413 commented Oct 12, 2022 •
edited

LGTM! Many thanks! @lsytj0413 wanna take a look?

There is a lot of enhancement in this PR, include many UnmarshalJSON implement. I'd prefer opening an issue for every fix
& improment to describe why we need this. That would help us understand more about it.

@spolti
Copy link
Member Author

spolti commented Oct 13, 2022

LGTM! Many thanks! @lsytj0413 wanna take a look?

There is a lot of enhancement in this PR, include many UnmarshalJSON implement. I'd prefer opening an issue for every fix & improment to describe why we need this. That would help us understand more about it.

Not really, the main goal is to fix the nil pointers. If you want to break it in different issues, I am not against but I don't see a strong reason to do it.

@lsytj0413
Copy link
Collaborator

Not really, the main goal is to fix the nil pointers. If you want to break it in different issues, I am not against but I don't see a strong reason to do it.

There is a lot change of files & lines, an issue still helpful.

@ricardozanini
Copy link
Member

Not really, the main goal is to fix the nil pointers. If you want to break it in different issues, I am not against but I don't see a strong reason to do it.

There is a lot change of files & lines, an issue still helpful.

Agreed.

sonatype-lift[bot]
sonatype-lift bot reviewed Oct 13, 2022
View reviewed changes
model/event_state_test.go Outdated Show resolved Hide resolved
sonatype-lift[bot]
sonatype-lift bot reviewed Oct 13, 2022
View reviewed changes
model/event_state_test.go Outdated Show resolved Hide resolved
@spolti
Copy link
Member Author

spolti commented Oct 13, 2022

@ricardozanini @lsytj0413 can you guys please take another look?

created this issue then: #107

@lsytj0413 lsytj0413 mentioned this pull request Oct 13, 2022
Merged
ricardozanini
ricardozanini reviewed Oct 14, 2022
View reviewed changes
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.

@spolti I think there's only one comment left to deal before merging this. timeouts is optional, we should return nil.

@spolti spolti mentioned this pull request Oct 14, 2022
Closed
@spolti
review changes
a09e387 
Signed-off-by: spolti <filippespolti@gmail.com>
@spolti
correct timeout declaration
bb3f7ba 
Signed-off-by: spolti <filippespolti@gmail.com>
lsytj0413
lsytj0413 reviewed Oct 16, 2022
View reviewed changes
model/sleep_state.go Outdated Show resolved Hide resolved
model/sleep_state.go Outdated Show resolved Hide resolved
parser/testdata/workflows/greetings-v08-spec.sw.yaml Outdated Show resolved Hide resolved
@spolti
review updates
19ffab8 
Signed-off-by: spolti <filippespolti@gmail.com>
@spolti
Copy link
Member Author

spolti commented Oct 19, 2022

small fixes for #70

lsytj0413
lsytj0413 reviewed Oct 20, 2022
View reviewed changes
model/states.go Outdated Show resolved Hide resolved
model/states.go Outdated Show resolved Hide resolved
model/states.go Outdated Show resolved Hide resolved
model/states.go Outdated Show resolved Hide resolved
model/states.go Outdated Show resolved Hide resolved
model/workflow.go Outdated Show resolved Hide resolved
@spolti
review additions
cef8e7a 
Signed-off-by: spolti <filippespolti@gmail.com>
@lsytj0413
Copy link
Collaborator

/lgtm

@ricardozanini ricardozanini merged commit e83573b into serverlessworkflow:main Oct 20, 2022
@spolti spolti deleted the v08Test branch October 20, 2022 17:24
spolti added a commit to spolti/sdk-go that referenced this pull request Oct 28, 2022
@spolti
fix nill state timeouts and enhance v08 test (serverlessworkflow#104)
0174926 
fixes serverlessworkflow#110
fixes serverlessworkflow#126

Signed-off-by: spolti <filippespolti@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsytj0413 lsytj0413 lsytj0413 left review comments

@sonatype-lift sonatype-lift[bot] sonatype-lift left review comments

@ricardozanini ricardozanini ricardozanini approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@spolti @lsytj0413 @ricardozanini