Skip to content

doc/rbd: refine "Retrieving Image Information"#49340

Merged
colemitchell merged 1 commit intoceph:mainfrom
zdover23:wip-doc-2022-12-09-rbd-rados-rbd-cmds-retrieving-image-information-semantics
Dec 9, 2022
Merged

doc/rbd: refine "Retrieving Image Information"#49340
colemitchell merged 1 commit intoceph:mainfrom
zdover23:wip-doc-2022-12-09-rbd-rados-rbd-cmds-retrieving-image-information-semantics

Conversation

@zdover23
Copy link
Contributor

@zdover23 zdover23 commented Dec 9, 2022 •
edited
Loading

Refine the text and prompts in "Retrieving Image Information" in doc/rbd/rados-rbd-cmds.rst.

https://tracker.ceph.com/issues/57001

Signed-off-by: Zac Dover zac.dover@gmail.com

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@zdover23 zdover23 requested a review from a team December 9, 2022 00:27
@zdover23 zdover23 requested a review from a team as a code owner December 9, 2022 00:27
anthonyeleven
anthonyeleven approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@anthonyeleven anthonyeleven left a comment

Choose a reason for hiding this comment

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

One comment, non-blocking but please consider.

doc/rbd: refine "Retrieving Image Information"
d418037 
Refine the text and prompts in "Retrieving Image Information" in
doc/rbd/rados-rbd-cmds.rst.

https://tracker.ceph.com/issues/57001

Signed-off-by: Zac Dover <zac.dover@gmail.com>
@zdover23 zdover23 force-pushed the wip-doc-2022-12-09-rbd-rados-rbd-cmds-retrieving-image-information-semantics branch from 51d310a to d418037 Compare December 9, 2022 00:45
@colemitchell colemitchell merged commit 7c1191d into ceph:main Dec 9, 2022
@zdover23
Copy link
Contributor Author

zdover23 commented Dec 9, 2022

#49350 - Quincy backport
#49351 - Pacific backport

idryomov
idryomov reviewed Dec 9, 2022
View reviewed changes
Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

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

@zdover23 @anthonyeleven @colemitchell I want to raise two process-related issues with the on-going "doc refining" work:

  • I don't think splitting these mostly minor changes into tiny, a couple dozen lines, PRs makes sense. I would prefer to see a single PR per page (.rst file) so that it could be reviewed once and consistency across all sections could be easily enforced.

  • This stuff is merged way too quickly. ceph/rbd team is a code owner for doc/rbd so a review is auto-requested but PRs are merged without waiting for it to happen. As it stands, this just spams people's inboxes -- even the backport PRs, let alone the original PR, are merged before anyone gets a chance to take a look.

doc/rbd/rados-rbd-cmds.rst
possible name for an RBD image, and such a name might (at the least) be
confusing. In the intrest of helping the reader of this documentation to
form a clear idea of the way that RBD images are named, we offer this
syntax: ``rbd -p {pool-name} | info {image-name}``.
Copy link
Contributor

@idryomov idryomov Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Where did | come from? It's going to be interpreted as a pipe to info by the shell...

Copy link
Contributor

@idryomov idryomov Dec 9, 2022

Choose a reason for hiding this comment

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

Also, might have been useful to link "Image, snap, group and journal specs" section of the man page (https://docs.ceph.com/en/latest/man/8/rbd/#image-snap-group-and-journal-specs) here.

Copy link
Contributor Author

@zdover23 zdover23 Dec 9, 2022

Choose a reason for hiding this comment

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

Where did | come from? It's going to be interpreted as a pipe to info by the shell...

It's true. It's true. This is my fault. I'll remove it in a follow-up PR.

Copy link
Contributor Author

@zdover23 zdover23 Dec 9, 2022

Choose a reason for hiding this comment

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

Also, might have been useful to link "Image, snap, group and journal specs" section of the man page (https://docs.ceph.com/en/latest/man/8/rbd/#image-snap-group-and-journal-specs) here.

I'll link to this in a future PR. One of the reasons that we have developed this accidentally irritating workflow is so that we can ingest suggestions like this.

As I said (or at least meant to) in another comment on this page, I'm sorry that our docs workflow spammed your inbox and made your workweek crummier than it otherwise would have been.

Copy link
Contributor

@idryomov idryomov Dec 9, 2022

Choose a reason for hiding this comment

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

No problem, ingesting suggestions is exactly what PR review is for ;) Doing this in after-merge comments and follow up PRs however is far from ideal, especially when the original PR has already been backported.

@zdover23
Copy link
Contributor Author

zdover23 commented Dec 9, 2022

@idryomov, I hear you. I'll make a single PR next week for https://docs.ceph.com/en/latest/rbd/rbd-snapshot/, and I hope that the single PR will keep your inbox from being flooded with docs-related notifications that aren't helpful.

I apologize for opening these PRs and then merging them as quickly as we have been. In almost every case, each of the docs PRs that we open concerns an issue that has been under discussion for hours and the PR represents the final phase of the discussion and revision. I'll insist that we keep these RBD PRs open for however long you ask. Does three days seem like a good amount of time for technical review?

@idryomov
Copy link
Contributor

idryomov commented Dec 9, 2022

I'll insist that we keep these RBD PRs open for however long you ask. Does three days seem like a good amount of time for technical review?

The proper workflow would be to keep them open until they are approved on behalf of ceph/rbd by a member of the RBD team. If you don't a get response for few days, leave a ping comment in the PR or reach out directly.

@zdover23
Copy link
Contributor Author

zdover23 commented Dec 9, 2022

I'll insist that we keep these RBD PRs open for however long you ask. Does three days seem like a good amount of time for technical review?

The proper workflow would be to keep them open until they are approved on behalf of ceph/rbd by a member of the RBD team. If you don't a get response for few days, leave a ping comment in the PR or reach out directly.

@idryomov, We'll do it that way.

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

Reviewers

@idryomov idryomov idryomov left review comments

@anthonyeleven anthonyeleven anthonyeleven approved these changes

Assignees

No one assigned

Labels

documentation rbd

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zdover23 @idryomov @anthonyeleven @colemitchell