Skip to content

Conversation

shreyasiddhartha
Copy link
Contributor

@shreyasiddhartha shreyasiddhartha commented Feb 12, 2025

Change type: Doc update; OpenShift Service Mesh Console (OSSMC) plugin Integration

Doc JIRA: https://issues.redhat.com/browse/OSSM-6063

Fix Version: service-mesh-docs-main and service-mesh-docs-3.0

Doc Preview: https://88446--ocpdocs-pr.netlify.app/openshift-service-mesh/latest/observability/kiali/ossm-console-plugin

SME Review/QE Review: @jshaughn @ferhoyos
Peer Review: @jeana-redhat

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Feb 12, 2025

🤖 Tue Feb 25 11:39:44 - Prow CI generated the docs preview:
https://88446--ocpdocs-pr.netlify.app
Complete list of updated preview URLs: artifacts/updated_preview_urls.txt

@shreyasiddhartha
Copy link
Contributor Author

/label service-mesh

Copy link

@jshaughn jshaughn 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, I had one trivial and optional suggestion. But not blocking approval.

Copy link

@ferhoyos ferhoyos left a comment

Choose a reason for hiding this comment

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

LGTM

@shreyasiddhartha
Copy link
Contributor Author

/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 Feb 14, 2025
@jeana-redhat jeana-redhat added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Feb 14, 2025
Copy link

@ferhoyos ferhoyos left a comment

Choose a reason for hiding this comment

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

@shreyasiddhartha I suggests a more descriptive compatibility table to indicate version alignment betwen Kiali/OSSMC and OSSM and the OCP versions supported.

The table is in markdown format, sorry. I hope it is not difficult to convert it to adoc.

Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

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

Global comment: "OpenShift" is not really an approved short name. Instances of {ocp-short-name} in this PR should probably all be {ocp-product-title}. I corrected a few but will stop because it will be a lot of comments - please replace them throughout.

Overall this is pretty solid - there are some similar formatting issues across a big PR, so it' a decent amount of comments, but in pretty good shape regardless 🙂

/remove-label peer-review-in-progress
/remove-label peer-review-needed
/label peer-review-done

@openshift-ci openshift-ci bot 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 Feb 14, 2025
Copy link

@ferhoyos ferhoyos left a comment

Choose a reason for hiding this comment

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

@shreyasiddhartha I have added some comments about installation section and compatibility matrix

@shreyasiddhartha
Copy link
Contributor Author

@ferhoyos Can you please give your lgtm again?

Copy link

@ferhoyos ferhoyos left a comment

Choose a reason for hiding this comment

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

LGTM

@shreyasiddhartha
Copy link
Contributor Author

/remove-label peer-review-done

@openshift-ci openshift-ci bot removed the peer-review-done Signifies that the peer review team has reviewed this PR label Feb 24, 2025
@shreyasiddhartha
Copy link
Contributor Author

/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 Feb 24, 2025
@shreyasiddhartha
Copy link
Contributor Author

I have added the peer review label again as there were multiple changes after the previous peer review.

@jeana-redhat jeana-redhat added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Feb 24, 2025
Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

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

Glad to re-review - this looks great overall! There are still some issues with unapproved product name use.

"OpenShift" by itself is not an approved name for OpenShift Container Platform. I am not sure the history behind the {ocp-short-name} attribute, but we are not supposed to use "OpenShift" as a short form.

I have replaced {ocp-short-name} with suggestions of {ocp-product-title} where I see it in this PR, but honestly most of them can probably just be removed. Saying "the web console" is typically sufficient without specifying the product (in most contexts).

/remove-label peer-review-in-progress
/remove-label peer-review-needed
/label peer-review-done

{SMPluginShort} is only supported on {ocp-short-name} 4.15 and above. For {ocp-short-name} 4.14 users, only the standalone Kiali console is accessible.
====

You can install the {SMPluginShort} by using the {ocp-product-title} web console or the {oc-first}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - apparently the oc-first attribute isn't in the service mesh attributes file. If you want to add it to your file for use, the format is a little weird:

:oc-first: pass:quotes[OpenShift CLI (`oc`)]

Otherwise, you can just type out the thing when you need it (I find that annoying to type, hence created the attribute 😉)

Suggested change
You can install the {SMPluginShort} by using the {ocp-product-title} web console or the {oc-first}.
You can install the {SMPluginShort} by using the {ocp-product-title} web console or the OpenShift CLI (`oc`).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jeana-redhat! I removed the attribute because it wasn't displaying correctly. I will try adding the attribute and see what happens again. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

it's working, yay 🎉

@openshift-ci openshift-ci bot 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 Feb 24, 2025
Updates after peer review 2
Copy link

openshift-ci bot commented Feb 25, 2025

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

@shreyasiddhartha
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Feb 25, 2025
@jeana-redhat jeana-redhat added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Feb 25, 2025
Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

/remove-label merge-review-in-progress
/remove-label merge-review-needed

@openshift-ci openshift-ci bot removed merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR labels Feb 25, 2025
@jeana-redhat jeana-redhat merged commit 45fa19a into openshift:service-mesh-docs-main Feb 25, 2025
2 checks passed
@jeana-redhat
Copy link
Contributor

/cherrypick service-mesh-docs-3.0

@openshift-cherrypick-robot

@jeana-redhat: new pull request created: #89162

In response to this:

/cherrypick service-mesh-docs-3.0

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

peer-review-done Signifies that the peer review team has reviewed this PR service-mesh Label for all Service Mesh 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