Simplify config parsing #1643
Simplify config parsing #1643
Conversation
k8s-ci-robot
added
the
kind/cleanup
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
approved
saschagrunert
force-pushed
the
config-simplify
branch
from
f607758 to
891b378
Compare
k8s-ci-robot
added
the
size/L
saschagrunert
force-pushed
the
config-simplify
branch
from
891b378 to
6156660
Compare
k8s-ci-robot
added
the
do-not-merge/hold
saschagrunert
force-pushed
the
config-simplify
branch
from
6156660 to
97fd761
Compare
| // YAML representation contains 2 yaml ScalarNodes per config option. | ||
| // One is config option name and other is the value of the option | ||
| // These ScalarNodes help preserve comments associated with | ||
| // the YAML entry |
There was a problem hiding this comment.
I don't understand this comment. Why and where the comments associated with the YAML entry was ever used?
There was a problem hiding this comment.
besides this comment it is lgtm
There was a problem hiding this comment.
Hm, I think the issue is that we now do not preserve yaml comments any more. This is an issue, right?
There was a problem hiding this comment.
Looks like we cannot preserve the comments without the current logic. Closing this PR for now.
There was a problem hiding this comment.
I am not sure where we are preserving comments. Aren't config is just readonly?
If we do preserver comments in some scenario - it will be great to have a test for it so it is not just a comment.
There was a problem hiding this comment.
We preserve the comments but not the newlines right now, I added an e2e test scenario for the whole crictl config subcommand in #1659
We can assume that the config is a flat map of keys and values and manually merge them into the `Config` struct. This allows to avoid manually parsing the YAML. Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert
force-pushed
the
config-simplify
branch
from
97fd761 to
6ff82ef
Compare
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We can assume that the config is a flat map of keys and values and manually merge them into the
Configstruct. This allows to avoid manually parsing the YAML.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?