Skip to content

[SRVLOGIC-469]: Deploying workflows chapter does not use kn worklow plugin #85916

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

Open
wants to merge 12 commits into
base: serverless-docs-main
Choose a base branch
from

Conversation

domhanak
Copy link

@domhanak domhanak commented Dec 6, 2024

Now it uses the kn workflow plugin

Version(s):

serverless-docs-1.34
serverless-docs-1.35

Issue:

SRVLOGIC-269

Link to docs preview:

Preview

QE review:

  • QE has approved this change.

Additional information:

@kaldesai @wmedvede @ricardozanini PTAL

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 6, 2024
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 6, 2024
Copy link

openshift-ci bot commented Dec 6, 2024

Hi @domhanak. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@domhanak
Copy link
Author

Need to also fix the devmode and the other referenced guide. Ty for review, so far will ping you when I push the changes

@kaldesai
Copy link

kaldesai commented Jan 9, 2025

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 9, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jan 9, 2025

Copy link

@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.

A minor suggestions. Otherwise, LGTM!

@kaldesai
Copy link

kaldesai commented Jan 15, 2025

@domhanak Once you have drafted the content, you can assign this PR for the peer review process by adding a /label peer-review-needed label to it.

Note: Make sure you have squashed down the commit to only 1.

The OpenShift peer review squad will then peer review this PR, and you can proceed with the merge review process.

domhanak and others added 6 commits January 15, 2025 08:24
Review from Kalyani

Co-authored-by: Kalyani Desai <[email protected]>
Reorder docs to make more sense and follow the precedures
from basic (what user should do 1st, to more complex ones )
Removes the example workflow from the module and replaces it with
prerequsites that user should have the WF deployed.
Adds steps to query all services.
@kaldesai
Copy link

@domhanak You want me to put this on the peer review as well?

Unify namespace usage.
Use shorthand -n where possible.
Add Testing section to deploying-workflows-dev-mode
@domhanak
Copy link
Author

@kaldesai not yet I still feel like some parts can be enhanced. I ll ping you :)

Copy link

openshift-ci bot commented Jan 16, 2025

@domhanak: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@kaldesai
Copy link

@domhanak Do you want me to put this on the peer review as well?

@domhanak
Copy link
Author

@kaldesai if you think it is ok please do

@kaldesai
Copy link

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Jan 22, 2025
@adellape adellape self-assigned this Jan 22, 2025
@adellape adellape added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Jan 22, 2025
----
+
.Example command for the `greetings-workflow.yaml` file:
.Example command for a workflow that has ID attribute with value `greeting`:
Copy link
Contributor

@adellape adellape Jan 22, 2025

Choose a reason for hiding this comment

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

No colons for titles:

Suggested change
.Example command for a workflow that has ID attribute with value `greeting`:
.Example command for a workflow that has an ID attribute with value `greeting`

$ oc logs pod/<pod-name> -n <namespace>
----

.Cleanup
Copy link
Contributor

@adellape adellape Jan 22, 2025

Choose a reason for hiding this comment

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

Technically .Cleanup is a non-standard heading for a procedure module (per modular docs guide). FWIW, I see only 1 other instance in the repo (though, that one has it as .Clean up two-words; the single-word here is correctly what IBM Style Guide suggests).

Could this instead just be the last step in the normal .Procedure? Or maybe the last step in .Verification? Alternatively, could be split out into its own procedure module "Cleaning up a workflow deployment" but that seems like overkill.

----

.Cleanup
. To cleanup the deployment, delete the projects resources running the following command:
Copy link
Contributor

@adellape adellape Jan 22, 2025

Choose a reason for hiding this comment

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

Either way though, if this stays as a single-step procedure, use an unordered list (per guidelines) and use "by running":

Suggested change
. To cleanup the deployment, delete the projects resources running the following command:
* To cleanup the deployment, delete the projects resources by running the following command:

. To deploy the application, apply the YAML file by entering the following command:
. In your terminal, navigate to the root directory of your {ServerlessLogicProductName} project.

. Deploy the application, by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Deploy the application, by running the following command:
. Deploy the application by running the following command:

----

.Testing
Copy link
Contributor

@adellape adellape Jan 22, 2025

Choose a reason for hiding this comment

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

Same thought re: non-standard heading for this .Testing one. This one, however, feels like splitting it out as its own procedure would be worthwhile (versus including it within some other procedure module).

Edit: I see now there already is a "Testing a deployment" procedure later in the assembly. Is that basically the same procedure that's being shown here? If so, could there just be an "Additional resources" link to that procedure from this one instead of duplicating?

+
Note the value of `HOST/PORT` from the result of tis command, referenced as `<WORKFLOW_URL>` in next step.

.. Navigate to the `<WORKFLOW_URL>/q/dev-ui/org.apache.kie.sonataflow.sonataflow-quarkus-devui/workflows` in your browser. Please note that it may take some time until the management console is available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Re: previous comment, match lowercase here as well, if you change the other:

Suggested change
.. Navigate to the `<WORKFLOW_URL>/q/dev-ui/org.apache.kie.sonataflow.sonataflow-quarkus-devui/workflows` in your browser. Please note that it may take some time until the management console is available.
.. Navigate to the `<workflow_url>/q/dev-ui/org.apache.kie.sonataflow.sonataflow-quarkus-devui/workflows` in your browser. Please note that it may take some time until the management console is available.

+
Note the value of `HOST/PORT` from the result of tis command, referenced as `<WORKFLOW_URL>` in next step.

.. Navigate to the `<WORKFLOW_URL>/q/dev-ui/org.apache.kie.sonataflow.sonataflow-quarkus-devui/workflows` in your browser. Please note that it may take some time until the management console is available.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Don't need "please", or maybe even "note".
  • "might" instead of "may" (better for translation).
Suggested change
.. Navigate to the `<WORKFLOW_URL>/q/dev-ui/org.apache.kie.sonataflow.sonataflow-quarkus-devui/workflows` in your browser. Please note that it may take some time until the management console is available.
.. Navigate to the `<WORKFLOW_URL>/q/dev-ui/org.apache.kie.sonataflow.sonataflow-quarkus-devui/workflows` in your browser. It might take some time until the management console is available.


.. Navigate to the `<WORKFLOW_URL>/q/dev-ui/org.apache.kie.sonataflow.sonataflow-quarkus-devui/workflows` in your browser. Please note that it may take some time until the management console is available.


.Next steps
Copy link
Contributor

Choose a reason for hiding this comment

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

This single-step .Next steps procedure could probably just be absorbed into wherever the .Testing procedure above it ends up.

@@ -13,62 +13,22 @@ You can verify that your {ServerlessLogicProductName} workflow is running by per
* You have an {ServerlessLogicOperatorName} installed on your cluster.
* You have access to an {ServerlessLogicProductName} project with the appropriate roles and permissions to create applications and other workloads in {ocp-product-title}.
* You have installed the OpenShift CLI `(oc)`.
* You have deployed your workflow in preview mode in the {ocp-product-title}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop "the":

Suggested change
* You have deployed your workflow in preview mode in the {ocp-product-title}.
* You have deployed your workflow in preview mode in {ocp-product-title}.


.Procedure

. Create a workflow `YAML` file similar to the following:
. Retrieve the list of service and identify the one matching `name` of your workflow:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be plural?

Suggested change
. Retrieve the list of service and identify the one matching `name` of your workflow:
. Retrieve the list of services and identify the one matching the `name` of your workflow:

@adellape adellape added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels Jan 22, 2025
@adellape adellape added this to the Continuous Release milestone Jan 22, 2025
@adellape adellape added the serverless Label for all Serverless PRs label Jan 22, 2025
@domhanak
Copy link
Author

domhanak commented Feb 12, 2025

@adellape You made me think hard about the guide and how it looks as whole. I am trying to improve it

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. peer-review-done Signifies that the peer review team has reviewed this PR serverless Label for all Serverless PRs size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants