Skip to content

Conversation

matthias-pichler
Copy link
Collaborator

@matthias-pichler matthias-pichler commented Jun 3, 2024

Signed-off-by: Matthias Pichler [email protected]

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Use Cases
  • Community
  • CTK
  • Other

Discussion or Issue link:

What this PR does:

I adressed some issues with the workflow schema:

  1. I used const instead of constant as described here: https://json-schema.org/understanding-json-schema/reference/const
  2. I added some missing type: object directives that made the schema fail certain strict schema checks (specifically ajv complained about the use of properties without type: object)
  3. I allowed tasks to reference authentication policies defined under use by allowing string values for authentication fields of tasks
  4. I replaced one erroneous use of ref with the correct $ref

Additional information:

the schema is very inconsistently formatted: would it be okay for me to configure yamllint and format the schema (in a separate PR?)

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! ❤️

@ricardozanini
Copy link
Member

the schema is very inconsistently formatted: would it be okay for me to configure yamllint and format the schema (in a separate PR?)

@matthias-pichler-warrify please do! 🙏

Can you also please add a GHA to validate if the YAML is formatted corrected? Also, I think we have a vscode configuration file that we can use for Contributors to configure their local dev.

So please, don't forget to add a section in CONTRIBUTIONS.md about how to lint. <3

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.

Can you please add examples that validate these changes? 🙏

@ricardozanini ricardozanini changed the title Fix workflow schema Fix workflow schema: change const, adding missing object; etc. Jun 3, 2024
@ricardozanini ricardozanini added change: fix Something isn't working. Impacts in a minor version change. area: spec Changes in the Specification labels Jun 3, 2024
@matthias-pichler
Copy link
Collaborator Author

the schema is very inconsistently formatted: would it be okay for me to configure yamllint and format the schema (in a separate PR?)

@matthias-pichler-warrify please do! 🙏

Can you also please add a GHA to validate if the YAML is formatted corrected? Also, I think we have a vscode configuration file that we can use for Contributors to configure their local dev.

So please, don't forget to add a section in CONTRIBUTIONS.md about how to lint. <3

will do it separately though

@matthias-pichler
Copy link
Collaborator Author

Can you please add examples that validate these changes? 🙏

I added examples to validate the last two points ... the first two are a matter of the schema itself so a different validation would have to be added.

Signed-off-by: Matthias Pichler <[email protected]>
@ricardozanini ricardozanini merged commit cad3d64 into serverlessworkflow:main Jun 3, 2024
@matthias-pichler matthias-pichler deleted the fix-schema branch June 3, 2024 19:07
@matthias-pichler
Copy link
Collaborator Author

the schema is very inconsistently formatted: would it be okay for me to configure yamllint and format the schema (in a separate PR?)

@matthias-pichler-warrify please do! 🙏

Can you also please add a GHA to validate if the YAML is formatted corrected? Also, I think we have a vscode configuration file that we can use for Contributors to configure their local dev.

So please, don't forget to add a section in CONTRIBUTIONS.md about how to lint. <3

working on this here: #883

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: fix Something isn't working. Impacts in a minor version change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants