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

doc: replace spaces with underscores in config option names #45887

Merged
merged 2 commits into from
Apr 23, 2022

Conversation

bluikko
Copy link
Contributor

@bluikko bluikko commented Apr 13, 2022

Several documentation files use the historical format of
spaces in config parameter names instead of underscores.
Change them to use underscores instead to follow the
current practice.

There was also a malformed config with both space and
an underscore and a missing newline at the last line of
demo config.

Monitoring OSD section had incorrect text wrapping in
one occurrence and several occurrence of extra spaces:
double or even triple spaces between words and extra
spaces at end of lines on a diagram.
Python API section had a config example without spaces
around the equal sign.

Signed-off-by: Ville Ojamo 14869000+bluikko@users.noreply.github.com

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

@bluikko
Copy link
Contributor Author

bluikko commented Apr 13, 2022

It would seem GitHub had automatically added a newline at the end since the last line was missing a newline.
I wonder if that lack of newline was intentional?

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.

Nice catch. I can't imagine a reason why the lack of a terminal newline would be intentional.

@bluikko
Copy link
Contributor Author

bluikko commented Apr 13, 2022

Furthermore, I just noticed there are more strange looking config in this file:

osd_pool_default_min size = {n} # Allow writing n copy in a degraded state.
osd_pool_default_pg num = {n}
osd_pool_default_pgp num = {n}

Are all these 3 having the final underscore _ in the parameter name changed into a space for some reason?!

Edit: indeed all 3 config parameters would seem to be having an underscore instead of a space. Will add a commit for that I guess?

@anthonyeleven
Copy link
Contributor

Another nice catch. Last year I went around and changed all the spaces in option names that I could find to be underscores, for uniformity and easy of copy/paste. Clearly I missed these. Please do add / amend for these changes.

@bluikko
Copy link
Contributor Author

bluikko commented Apr 13, 2022

Another nice catch. Last year I went around and changed all the spaces in option names that I could find to be underscores, for uniformity and easy of copy/paste. Clearly I missed these. Please do add / amend for these changes.

Ah, that makes sense. I am new to ceph, still middle of first production deployment, so was unaware that the syntax takes both spaces/underscores and the history. Pushed another commit for those 3 lines.

@anthonyeleven
Copy link
Contributor

Welcome, and thanks for contributing.

@bluikko
Copy link
Contributor Author

bluikko commented Apr 13, 2022

Please excuse me, since this question is out of scope of this PR - but wouldn't osd_journal_size be concerning only filestore and thus should be dropped out completely from this file?

@anthonyeleven
Copy link
Contributor

By all means please do question anything and everything with fresh eyes. I suspect that this file has been with us for a long time. I poked around to see if it were depended upon by other resources that might have cause to deploy Filestore OSDs, but best I can tell it is truly a standalone example of what ceph.conf is like, nothing more and nothing less.

Many operators do still have pre-existing Filestore OSDs, and a few have specific reasons to continue deploying them, so my sense is that it's not yet time to erase all traces of it from the docs. That said, I agree with you that osd_journal_size probably is at best confusing in this file, and encourage you to amend your commit to remove that line.

@bluikko
Copy link
Contributor Author

bluikko commented Apr 20, 2022

As I read the documentation I keep stumbling to more spaces in configuration variable names such as:

It seems to me that the above sections should be fixed to use underscores if that is the current policy.
Then this MR title should be updated to be more generic.

Would you squash on the possible merge or should I squash after adding the commits for other documentation files?

@anthonyeleven
Copy link
Contributor

Nice catch. I think we have sufficient precedent to prefer underscores throughout the docs.

Squash and merge appears to not be enabled for this repository, so it'd be best for you to amend/squash/force-push.

@github-actions github-actions bot added the rbd label Apr 20, 2022
@bluikko
Copy link
Contributor Author

bluikko commented Apr 20, 2022

Nice catch. I think we have sufficient precedent to prefer underscores throughout the docs.

Squash and merge appears to not be enabled for this repository, so it'd be best for you to amend/squash/force-push.

I will keep on adding commits as I see such config in the docs and squash in the end, possibly near the end of the week. Thanks.

@bluikko bluikko changed the title doc: space in config parameter in demo-ceph doc: config parameter spaces to underscores Apr 20, 2022
@idryomov idryomov changed the title doc: config parameter spaces to underscores doc: replace spaces with underscores in config option names Apr 20, 2022
@idryomov
Copy link
Contributor

I will keep on adding commits as I see such config in the docs and squash in the end, possibly near the end of the week.

@bluikko I renamed the PR, squashed and was about to merge when I noticed that you were looking for more occurrences. Underscores are indeed preferred, let me know when it's ready!

@bluikko
Copy link
Contributor Author

bluikko commented Apr 22, 2022

Indeed, was going to finish this by the end of the week.

@bluikko
Copy link
Contributor Author

bluikko commented Apr 22, 2022

There are often double spaces, more often after punctuation than between words. Is that on purpose? There is quite a bit of them!

Edit: I am not very familiar with RST but at a quick look I could not find any syntactic meaning for it. Perhaps something for another PR?

Also quite a few inconsistencies with

``varname``

vs

:confval:`varname`

where the former should probably be changed to latter, e.g. https://docs.ceph.com/en/latest/rados/operations/pools/

Some formatting oopsie at https://docs.ceph.com/en/latest/rados/api/python/#rados.Ioctx.get_stats for num_objects_missing_on_primary, but I need to check where the automethod content comes from...

@bluikko
Copy link
Contributor Author

bluikko commented Apr 22, 2022

That does it for "Ceph Storage Cluster" chapter. Instead of taking list of config var names, replacing _ with a space and loop that for a recursive grep, I scanned manually - the purpose was to learn something about Ceph and make a list of what to read later.

I will think whether automating the rest would make more sense... target is still end of the week.

@anthonyeleven
Copy link
Contributor

anthonyeleven commented Apr 22, 2022

I support the manual idea, or at least a review of results before committing. I would expect some misleading hits from a fully automated action.

Alternately we can merge what we have and you could follow up with another PR. The longer we delay the greater the chance of having to rebase.

@bluikko
Copy link
Contributor Author

bluikko commented Apr 23, 2022

I support the manual idea, or at least a review of results before committing. I would expect some misleading hits from a fully automated action.

Alternately we can merge what we have and you could follow up with another PR. The longer we delay the greater the chance of having to rebase.

I think your suggestion is probably the best way, conflicts should be avoided of course.

With that in mind, this PR is ready.
Future fixes I would then plan first and commit in a short time span and not use a week for it.

I was planning that if doing some kind of scripting then it would be only for detection of spaces instead of underscores. All fixing I would have planned to do manually.

Signed-off-by: Ville Ojamo <14869000+bluikko@users.noreply.github.com>
Signed-off-by: Ville Ojamo <14869000+bluikko@users.noreply.github.com>
@idryomov
Copy link
Contributor

With that in mind, this PR is ready.

One underscore in osd_crush_chooseleaf_type was still missing. Otherwise LGTM!

@idryomov idryomov merged commit 29941e3 into ceph:master Apr 23, 2022
@anthonyeleven
Copy link
Contributor

There are often double spaces, more often after punctuation than between words. Is that on purpose? There is quite a bit of them!

Many of us were taught to write that way during the typewriter era and even beyond. It's been an ingrained practice for quite some time. In recent years there has been movement toward single spaces after punctuation. A lot of software these days ends up rendering single and double spaces after punctuation identically anyway, so in our context while I think we currently prefer a single space, I personally don't consider a double space an error.

Edit: I am not very familiar with RST but at a quick look I could not find any syntactic meaning for it. Perhaps something for another PR?

As far as I'm concerned, collapsing double spaces doesn't itself warrant a PR, but if you're already in a file making other changes it's okay to make those too, but I wouldn't spent any time on it.

Also quite a few inconsistencies with

``varname``

vs

:confval:`varname`

where the former should probably be changed to latter, e.g. https://docs.ceph.com/en/latest/rados/operations/pools/

My sense is that the latter may only have been valid and functional within the last year or two, so there are many pre-existing instances. My personal practice lately has been to update when I'm making other changes.

Some formatting oopsie at https://docs.ceph.com/en/latest/rados/api/python/#rados.Ioctx.get_stats for num_objects_missing_on_primary, but I need to check where the automethod content comes from...

That one's beyond my ken, unfortunately.

@bluikko
Copy link
Contributor Author

bluikko commented Apr 25, 2022

@anthonyeleven Very clear, thank you!

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

Successfully merging this pull request may close these issues.

3 participants