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

Consolidate the YAML parsing into libnetplan: low-hanging fruits (FR-702) #241

Merged
merged 3 commits into from
Jan 10, 2022

Conversation

schopin-pro
Copy link
Contributor

@schopin-pro schopin-pro commented Oct 25, 2021

Description

Until now, we parsed the YAML configuration at different places in the program:

  • libnetplan, where we have most of our business logic, and where we implement data verification
  • the netplan set command, where we parse the YAML tree to do some syntactical merging of the trees without looking too much into the semantics
  • configmanager.py + sriov.py where we have some business logic to be able to configure the SRIOV backend on the fly.
  • netplan migrate, which isn't supposed to be used in normal circumstances

Additionally, we use PyYAML in netplan info to easily generate well-formed YAML data from a Python dict.

This PR removes the migrate and info use cases, the other cases will have their own PRs in order to keep review complexity somewhat manageable.

Why?

There are a couple of advantages to this new approach:

  • Single point of validation: whenever we want to parse some configuration, we'll be able to validate the new configuration in a consistent manner
  • Smaller dependency tree: netplan is intended for all sorts of use-case, including some where disk size is a real constraint. python3-yaml has an unpacked size of 0.5 Mo, which can now be shaved off.
  • Better APIs: this is more of a by-product, but doing this forced us to implement proper APIs for libnetplan ;-)

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).

@schopin-pro schopin-pro force-pushed the yaml-consolidation branch 2 times, most recently from 7bd588d to 800082a Compare October 25, 2021 10:55
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2021

Codecov Report

Merging #241 (7c8729f) into main (e0a07bc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #241   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files          58       58           
  Lines       10052    10055    +3     
=======================================
+ Hits         9962     9965    +3     
  Misses         90       90           
Impacted Files Coverage Δ
netplan/cli/commands/info.py 100.00% <100.00%> (ø)
netplan/cli/commands/migrate.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0a07bc...7c8729f. Read the comment docs.

@schopin-pro schopin-pro force-pushed the yaml-consolidation branch 5 times, most recently from 00259c7 to 2ae70d1 Compare November 16, 2021 19:33
@schopin-pro
Copy link
Contributor Author

I've reworked the code to use file descriptors where it made sense instead of giving out file names, and the code coverage should reach 100% (with some error paths excluded as really hard to trigger in the tests). I also added some documentation for the non-trivial new APIs.

Now all that remains (besides merging #233) is the reviewability issue.

@schopin-pro schopin-pro force-pushed the yaml-consolidation branch 3 times, most recently from 9dd8933 to 169547d Compare November 17, 2021 13:16
@schopin-pro schopin-pro force-pushed the yaml-consolidation branch 3 times, most recently from 6670f1a to 3287c5d Compare January 5, 2022 14:08
@schopin-pro schopin-pro changed the title Consolidate the YAML parsing into libnetplan (FR-702) Consolidate the YAML parsing into libnetplan: low-hanging fruits (FR-702) Jan 5, 2022
@schopin-pro
Copy link
Contributor Author

This PR has just undergone a drastic diet to get things moving, and is now just the low hanging fruits, the main course will come in subsequents PRs. As such, I'm removing the draft status and move it up to review.

@schopin-pro schopin-pro marked this pull request as ready for review January 5, 2022 14:15
@schopin-pro schopin-pro requested a review from slyon January 5, 2022 14:20
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

This is a very nice PR description! Thank you, LGTM.
I left a tiny nitpick inline, feel free to merge regardless.

netplan/cli/commands/info.py Outdated Show resolved Hide resolved
schopin-pro and others added 3 commits January 10, 2022 09:32
The data is simple enough that it's reasonable to format it by hand
rather than relying on an external dependency.
Co-authored-by: Lukas Märdian <slyon@ubuntu.com>

Co-authored-by: Lukas Märdian <slyon@ubuntu.com>
@schopin-pro schopin-pro merged commit 519fb50 into canonical:main Jan 10, 2022
@schopin-pro schopin-pro deleted the yaml-consolidation branch January 13, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants