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

chore: remove --docs option from cloud-init schema #5857

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TheRealFalcon
Copy link
Member

Proposed Commit Message

chore: remove `--docs` option from `cloud-init schema`

It has been broken and has marginal value, so it is not worth
maintaining. Since it was a developer-facing option, this should
have no backwards compatibility concerns.

Fixes GH-5756

Additional Context

Test Steps

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

It has been broken and has marginal value, so it is not worth
maintaining. Since it was a developer-facing option, this should
have no backwards compatibility concerns.

Fixes canonicalGH-5756
Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

Big thanks for this cleanup @TheRealFalcon.

I haven't had a chance to look at this in depth, but it looks like we lost some coverage. That isn't to say that the code is lower quality, but without looking further it does make me wonder if this means that there is some dead code that we missed cleaning up that is now untested because we removed its test coverage. Thoughts?

Coverage of the new commit

Name                                                           Stmts   
cloudinit/config/schema.py                                       603     81    242     32    83%
cloudinit/importer.py                                             42      0     26      3    96%

Coverage of the previous commit

Name                                                           Stmts   

cloudinit/config/schema.py                                       797     59    344     34    91%
cloudinit/importer.py                                             42      0     26      1    99%

@TheRealFalcon
Copy link
Member Author

Outside of a line or two here or there, there seem to be 2 functions that no longer have coverage in schema.py:

  • _flatten(xs) appears to be dead
  • flatten_schema_refs(...) was used by some of the deleted code, but is still used in conf.py when building the documentation.

Because it was used by the removed code, the tests that tested the removed code also happened to remove the coverage for this function. I can add in new coverage for it.

@holmanb
Copy link
Member

holmanb commented Nov 12, 2024

* `_flatten(xs)` appears to be dead

Agreed - want to smite that too?

* `flatten_schema_refs(...)` was used by some of the deleted code, but is still used in conf.py when building the documentation.

Because it was used by the removed code, the tests that tested the removed code also happened to remove the coverage for this function. I can add in new coverage for it.

What if we just move it to conf.py? It isn't used by any cloud-init runtime code, so I would never put it where it is currently if adding it as new code. Also, it wouldn't look out of place alongside the function definitions in conf.py where it gets called - it's doing similar types of things as the current code that is there.

@github-actions github-actions bot added the documentation This Pull Request changes documentation label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This Pull Request changes documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants