YAML consolidation prelude: new YAML dump APIs (FR-702)#251
YAML consolidation prelude: new YAML dump APIs (FR-702)#251slyon merged 3 commits intocanonical:mainfrom
Conversation
|
Draft as it depends on #250 |
schopin-pro
changed the title
schopin-pro
force-pushed
the
yaml-consolidation-api-prelude
branch
from
c68271a to
b605144
Compare
schopin-pro
force-pushed
the
yaml-consolidation-api-prelude
branch
from
b605144 to
6920acc
Compare
Codecov Report
@@ Coverage Diff @@
## main #251 +/- ##
==========================================
+ Coverage 99.12% 99.13% +0.01%
==========================================
Files 60 60
Lines 10229 10404 +175
==========================================
+ Hits 10139 10314 +175
Misses 90 90
Continue to review full report at Codecov.
|
schopin-pro
force-pushed
the
yaml-consolidation-api-prelude
branch
2 times, most recently
from
bb22c20 to
086c4c8
Compare
schopin-pro
force-pushed
the
yaml-consolidation-api-prelude
branch
2 times, most recently
from
4744eb7 to
2496e6f
Compare
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thank you Simon! This is another great step in consolidating our YAML parsing story.
It is looking very good overall. I asked for some clarification and mentioned a few nitpicks in the inline comments. We should be good to merge this after those are resolved!
Also, I can confirm that the netplan_state_write_yaml() function isn't used anywhere and was never released. So we're good to "break" that API.
This allows for more flexibility than using filenames. The implementation is split in two functions to allow for future variants, i.e. updating an existing netplan configuration tree. This patch is technically an API break but the removed API was never released. V2: * use a helper function netplan_state_has_nondefault_globals() * fix the write_netplan_conf_full behavior on empty state to not create an empty file * DRASTICALLY simplify the implementation of netplan_state_dump_yaml(), fixing a leak by not using a new list in the first place
schopin-pro
force-pushed
the
yaml-consolidation-api-prelude
branch
from
2496e6f to
2815606
Compare
This API is also using file descriptors for flexibility. The idea is to be able to copy only part of a YAML document based on a "prefix", that is subsequent keys into YAML maps, separated by '\t' characters. V2: add some comments and use better names for the static helpers
schopin-pro
force-pushed
the
yaml-consolidation-api-prelude
branch
from
2815606 to
b10ff76
Compare
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thank you for addressing the comments, especially the netplan_state_nondefault_globals() function should avoid issues with our snapd integration so good idea going with option (3) there!
LGTM. Let's merge this.
3 participants
Description
Introduce a couple of APIs that are needed for the various YAML consolidation efforts. Note that this is technically an API break, but the function that is removed has never been part of a release, so this should be OK.
Checklist
make checksuccessfully.make check-coverage).