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

[KOGITO-7257] Documenting workflow metainfo #531

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Feb 8, 2024

@ricardozanini
Copy link
Member

@kaldesai can you take a look?

Copy link
Contributor

@kaldesai kaldesai left a comment

Choose a reason for hiding this comment

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

I have added a few suggestions. Otherwise, your content looks good to me. Thank you!

@fjtirado
Copy link
Contributor Author

Thanks a lot @kaldesai

In addition to the predefined keys mentioned previously, you can add your own keys to workflow context using Java Service Loader mechanism, by providing an implementation of class link:{kogito_runtimes_url}/kogito-serverless-workflow/kogito-serverless-workflow-utils/src/main/java/org/kie/kogito/serverless/workflow/utils/KogitoProcessContextResolverExtension.java[KogitoProcessContextResolverExtension]

This feature was used to add quarkus security identity support, you can check source code as reference link:{kogito_runtimes_url}/quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow/src/main/java/org/kie/kogito/serverless/workflow/QuarkusKogitoProcessContextResolver.java[here].

Copy link
Member

Choose a reason for hiding this comment

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

@domhanak based on your work to remove Quarkus citations throughout the doc sections, I think we should relocate this section Javi added. This is part of the "concepts" section, so we must not cite Quarkus here.

Copy link
Contributor Author

@fjtirado fjtirado Feb 13, 2024

Choose a reason for hiding this comment

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

@ricardozanini Since the quarkus mention is just an example of how to add a new context variable, I think the usage is legit here. The reason I mention the quarkus related implementation of KogitoProcessContextResolverExtension is because is straightforward and therefore suitable as example, but if an issue, I can change by a link to the one I used for python support

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should mention Quarkus here. It's highly related to what we are trying to do. These concepts sections must be solely to working with workflows at the spec level and how our runtime handles it. We can move these paragraphs to a new section under Quarkus. This is a rich material that we must keep, it's just a matter of rearrangement. @domhanak wdyt?

Copy link
Contributor

@domhanak domhanak Feb 14, 2024

Choose a reason for hiding this comment

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

@ricardozanini Agreed, I am noting this and can work on rearranging. Just let me know the order we do this.

Copy link
Member

Choose a reason for hiding this comment

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

I guess once we merge this, we can move in the PR you have opened #536

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.

We can merge like this and then rearrange and move as part of #536. @domhanak can you PTAL?

@fjtirado fjtirado merged commit f3972fc into apache:main Feb 15, 2024
1 check passed
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.

4 participants