-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
modules/update-service-overview: Simplify intro paragraph #26668
Conversation
82d96d1
to
ba1a04a
Compare
I'd initially wanted to address "It provides a graph, or diagram that contain" -> "It provides a graph, or diagram, that contains". But the "of component Operators" bit was confusing to me to. So I've reworded to lead with "graph of recommended update(s)", and reduce to a single sentence. I'd also be fine leading with a description of release images, and then introducing the recommended update edges that link them, but that seemed like a bigger refactor. Wording I'm replacing is from 29016d7 (draft of early 4.0 architecture updates, 2018-11-08, openshift#12880) and 6e1b894 (Initial add of modularized arch guide content, 2019-05-21, openshift#14991).
ba1a04a
to
e298686
Compare
The preview will be available shortly at: |
@jiajliu, will you PTAL? |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions/suggestions, but otherwise LGTM!
of the managed cluster components. | ||
updates to both {product-title} and {op-system-first}. It serves a graph of | ||
recommended update _edges_ between {product-title} release image _vertices_ that | ||
specify the intended state of the managed cluster components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what is specifying "the intended state of the managed cluster components?" It's a little confusing as to what "that" is referring to. For example:
It serves a graph of recommended update edges between OpenShift Container Platform release image vertices. The edges specify the intended state of the managed cluster components.
Also, is "provide" or "displays" more localization friendly versus "serves?"
Other than these questions/suggestions, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "vertices" specify the intended state of the managed cluster components, and it's important enough that I don't want to diminish it by changing it to "vertices, which..."
@wking, what nuance are you trying to get across with the s/provides/serves? "Serves" is used in the same context in other places in the collection, so I'm inclined to let it stand.
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closed this PR. In response to this:
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/test-infra repository. |
I'd initially wanted to address "It provides a graph, or diagram that contain" -> "It provides a graph, or diagram, that contains". But the "of component Operators" bit was confusing to me to. So I've reworded to lead with "graph of recommended update(s)", and reduce to a single sentence. I'd also be fine leading with a description of release images, and then introducing the recommended update edges that link them, but that seemed like a bigger refactor. Wording I'm replacing is from 29016d7 (#12880) and 6e1b894 (#14991).
/assign @kalexand-rh