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 #742] Id, key and name refactor #824

Merged
merged 4 commits into from
Mar 20, 2024
Merged

[Fix #742] Id, key and name refactor #824

merged 4 commits into from
Mar 20, 2024

Conversation

fjtirado
Copy link
Collaborator

@fjtirado fjtirado commented Feb 21, 2024 •
edited
Loading

Id is removed.
Name remains mandatory and is clarified that should be unique. Key description is updated and becomes optional in all cases. Version is assumed to be latest if not specified

Many thanks for submitting your Pull Request ❤️!

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Roadmap
  • Use Cases
  • Community
  • TCK
  • Other

Discussion or Issue link:

Fixes #742

What this PR does / why we need it:

Special notes for reviewers:

Additional information:

@fjtirado fjtirado force-pushed the Fix_#742 branch 2 times, most recently from 1a19c20 to 77c54a8 Compare February 21, 2024 13:30
@fjtirado
[Fix serverlessworkflow#742] Id, key and name refactor
3c9316b 
Id is removed.
Name remains mandatory and is clarified that should be unique.
Key description is updated and becomes optional in all cases.
Version is assumed to be latest if not specified

Signed-off-by: Francisco Javier Tirado Sarti <ftirados@redhat.com>
fjtirado
fjtirado commented Feb 21, 2024
View reviewed changes
specification.md Outdated Show resolved Hide resolved
ricardozanini
ricardozanini reviewed Feb 21, 2024
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.

Looks good! But there are a few snippets throughout the specification.md that need to be removed too. Can you search for id: or "id":

@ricardozanini ricardozanini mentioned this pull request Feb 21, 2024
Merged
9 tasks
@fjtirado
[Fix serverlessworkflow#742] Ricardos comments
77c77b2 
Signed-off-by: Francisco Javier Tirado Sarti <ftirados@redhat.com>
@fjtirado
Copy link
Collaborator Author

@cdavernas Can you please take a look?

cdavernas
cdavernas requested changes Mar 8, 2024
View reviewed changes
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Thank you so much for you work! ❤️

I've got some comments, though only a single one is a pita 😄

schema/auth.json Show resolved Hide resolved
schema/workflow.json Outdated Show resolved Hide resolved
schema/workflow.json Outdated Show resolved Hide resolved
schema/workflow.json Show resolved Hide resolved
@fjtirado fjtirado requested a review from cdavernas March 8, 2024 18:27
@fjtirado fjtirado force-pushed the Fix_#742 branch 3 times, most recently from e478795 to 00fc6c5 Compare March 8, 2024 18:43
cdavernas
cdavernas requested changes Mar 9, 2024
View reviewed changes
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Thank you for your work, once again!

I'm a pita, sorry man: I left a couple of small comments.

Also, could you be so kind to update the roadmap.md file to include changes of this PR?

schema/workflow.json Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
@fjtirado fjtirado requested a review from cdavernas March 11, 2024 10:34
cdavernas
cdavernas requested changes Mar 11, 2024
View reviewed changes
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
@fjtirado
[Fix serverlessworkflow#742] Charles comments
0e9e9cb 
Signed-off-by: Francisco Javier Tirado Sarti <ftirados@redhat.com>
@fjtirado
Copy link
Collaborator Author

@cdavernas I think I fixed all names misalignment on specification.md (hopefully ;))

@fjtirado fjtirado requested a review from cdavernas March 11, 2024 11:19
JBBianchi
JBBianchi reviewed Mar 11, 2024
View reviewed changes
specification.md Outdated Show resolved Hide resolved
JBBianchi
JBBianchi reviewed Mar 11, 2024
View reviewed changes
specification.md Outdated Show resolved Hide resolved
JBBianchi
JBBianchi reviewed Mar 11, 2024
View reviewed changes
specification.md Outdated Show resolved Hide resolved
@fjtirado fjtirado requested a review from JBBianchi March 11, 2024 11:46
@fjtirado
[Fix serverlessworkflow#742] Jean Baptiste comments
877816f 
Signed-off-by: Francisco Javier Tirado Sarti <ftirados@redhat.com>
JBBianchi
JBBianchi approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@JBBianchi JBBianchi left a comment

Choose a reason for hiding this comment

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

Great work @fjtirado ! Thanks a lot for your contribution.

@ricardozanini
Copy link
Member

@cdavernas wanna take a second look?

cdavernas
cdavernas approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@cdavernas cdavernas 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 @fjtirado!

@cdavernas
Copy link
Member

@ricardozanini Feel free to merge when ready: it has been approved by majority, and has been out for a month.

@ricardozanini ricardozanini merged commit e8ed64b into serverlessworkflow:main Mar 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JBBianchi JBBianchi JBBianchi approved these changes

@cdavernas cdavernas cdavernas approved these changes

@ricardozanini ricardozanini ricardozanini approved these changes

@tsurdilo tsurdilo Awaiting requested review from tsurdilo

@antmendoza antmendoza Awaiting requested review from antmendoza

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

Successfully merging this pull request may close these issues.

workflow.id and workflow.key is duplicate
4 participants
@fjtirado @ricardozanini @cdavernas @JBBianchi