YAML consolidation: netplan get (FR-702)
#252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 99.10% 99.13% +0.03%
==========================================
Files 58 60 +2
Lines 10038 10386 +348
==========================================
+ Hits 9948 10296 +348
Misses 90 90
Continue to review full report at Codecov.
|
This was referenced Jan 6, 2022
schopin-pro
force-pushed
the
yaml-consolidation-get
branch
3 times, most recently
from
88ee24f
to
5ab0d25
Compare
slyon
reviewed
Jan 19, 2022
There was a problem hiding this comment.
Thank you for another great PR, Simon!
The dirty tracking functionality is really nice and should help us cleaning up some parts of the test environment, too, where we're currently doing some "normalization" of default values.
I left some inline comments. But again, nothing critical.
src/netplan.c
Outdated
Comment on lines
640
to
644
| } else if (DIRTY(def, def->backend) || | ||
| ( def->backend != get_default_backend_for_type(np_state->backend, def->type) | ||
| && def->backend != np_state->backend | ||
| && def->backend != NETPLAN_BACKEND_OVS)) { | ||
| YAML_STRING_PLAIN(event, emitter, "renderer", netplan_backend_name(def->backend)); |
There was a problem hiding this comment.
Not to self: we should verify that this still works correctly with the NM snap integration. It should as parse-nm.c is setting each netdef's backend to NETPLAN_BACKEND_NM which is not equal to the default backend type.
This new structure, while more complex, will allow us to extend some callback signatures (i.e. adding a prefix parameter for null-handling) without having to change the whole world. V2: * simplify the map handling structure, getting rid of the tagged union in favor of simpler NULL-handling. * remove the fancy space alignment for the handler declarations as they got out of control.
This will allow us to output those fields even if their value is the default one, to clearly reflect the user's intent. V2: Fix conditional bracing style
If the user touched those fields, we want to retranscribe the value even if it matches the default! Squashed in this commit is a test fix for the on-link field. on-link is a boolean, and its output format was wrong. We want literals as much as possible! The behaviour change is entangled with the dirty changes, so we change the test here. V2: Change the macro names to remove the DIRTY_ bit and make the default values more explicit
schopin-pro
force-pushed
the
yaml-consolidation-get
branch
from
5ab0d25
to
877cc35
Compare
This patch rewrites the entire command to use our internal YAML helpers instead, notably the APIs introduced in the few previous commits. This also changes the tests to parse the output YAML instead of checking for a *specific* output, which makes them less susceptible of breaking when there's a small formatting change. V2: * add a test for the networkINVALID query * remove a spurious change in tests/generator/base.py
schopin-pro
force-pushed
the
yaml-consolidation-get
branch
from
877cc35
to
713a44b
Compare
|
Tests run on my local LXD, as GH ACtions seems to be having troubles today: |
slyon
approved these changes
Jan 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Port the
netplan getcommand to use the internal YAML generator, originally written for the NM-to-netplan system.In order to make this work, we had to add a notion of "dirtyness" to the input data, as the user would want their explicit settings displayed even if they match the default value.
Checklist
make checksuccessfully.make check-coverage).