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

OSSM-8759 Pre-migration checklist for product docs #88198

Merged

Conversation

gwynnemonahan
Copy link
Contributor

@gwynnemonahan gwynnemonahan commented Feb 6, 2025

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

@nrfox nrfox left a comment

Choose a reason for hiding this comment

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

Looks great overall! Just a few comments.

Copy link

@FilipB FilipB left a comment

Choose a reason for hiding this comment

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

Left a few comments

@FilipB
Copy link

FilipB commented Feb 10, 2025

@gwynnemonahan should we wait for missing links before approval or there will be another PR for adding links so it will be reviewed again there?

@gwynnemonahan
Copy link
Contributor Author

@gwynnemonahan should we wait for missing links before approval or there will be another PR for adding links so it will be reviewed again there?

Hey @FilipB ,

Thanks for asking!

At the moment, I'm OK with waiting on this PR. It is looking like this will be the last, or one of the last PRs for Migration content to be merged so that the links can be added and checked that they work. There's also a change I need to make regarding the checker script.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2025
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 17, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2025
Copy link

@FilipB FilipB left a comment

Choose a reason for hiding this comment

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

A few suggestions. Also we are still waiting for the links to be working before final approval.

Copy link

@nrfox nrfox left a comment

Choose a reason for hiding this comment

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

Couple formatting nits but otherwise LGTM

Copy link
Contributor

@michaelryanpeter michaelryanpeter left a comment

Choose a reason for hiding this comment

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

I think that this content is in between content types. As a result, it has an unfinished feeling to it.

I think that as a rule of thumb, a checklist should be part of an assembly and link/xref out to the relevant procedures. And if links are not necessary and the content is intended for reuse, the checklist should be a reference module.

For most of my comments, a slight refactoring of content to align that suggestion will solve the issues. But a few of these modules are procedure or reference modules (and not checklists), and should be used in that way.

That said, I don't it will take too much to get this into shape. Please reach out if you have questions. I am happy to hop on a call to discuss.

@michaelryanpeter michaelryanpeter 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 Mar 3, 2025
@gwynnemonahan gwynnemonahan force-pushed the OSSM-8759 branch 5 times, most recently from 7a5218d to 2037fb1 Compare March 4, 2025 04:27
@gwynnemonahan
Copy link
Contributor Author

Rebased night 03/03/2025.

@gwynnemonahan gwynnemonahan force-pushed the OSSM-8759 branch 2 times, most recently from 83d9131 to f20ecd6 Compare March 4, 2025 13:53
@gwynnemonahan
Copy link
Contributor Author

Rebased morning 03/04/2025

@gwynnemonahan
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 Mar 4, 2025
@skrthomas skrthomas added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Mar 4, 2025
Copy link
Contributor

@skrthomas skrthomas left a comment

Choose a reason for hiding this comment

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

Just a couple comments about minimalism here before merging.

Copy link

openshift-ci bot commented Mar 4, 2025

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

@skrthomas
Copy link
Contributor

I won't hold merge on this, but after some discussion with the reviewers of this PR and @kalexand-rh, we've decided that the best course of action is a re-evaluation of some of the minimalism and modularization comments in the review that weren't addressed, or were ACK'd pending user feedback. A Jira task has been created to ensure that this content is re-evaluated for consistency with these standards because, as is, this checklist does not align: https://issues.redhat.com/browse/OSSM-9002

@skrthomas skrthomas merged commit e4e0da1 into openshift:service-mesh-docs-main Mar 4, 2025
2 checks passed
@skrthomas
Copy link
Contributor

/cherrypick service-mesh-docs-3.0

@openshift-cherrypick-robot

@skrthomas: new pull request created: #89563

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
merge-review-in-progress Signifies that the merge review team is reviewing this PR 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.

10 participants