-
Notifications
You must be signed in to change notification settings - Fork 892
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
Conversation
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.
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%
Outside of a line or two here or there, there seem to be 2 functions that no longer have coverage in schema.py:
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. |
Agreed - want to smite that too?
What if we just move it to |
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
96010d5
to
b89ecff
Compare
Rebased and should be ready for re-review |
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.
Thanks! This looks good to me @TheRealFalcon. Feel free to merge as-is.
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
Proposed Commit Message
Additional Context
Test Steps
Merge type