search

uw-ictd / colte

Community LTE Project
MIT License
59 stars 27 forks source link

change CommentedMap to CommentedSeq when writing *.conf #95

Closed spencersevilla closed 3 years ago

spencersevilla commented 3 years ago

As part of the pdn -> subnet debugging I figured out that if the value doesn't exist in the open5gs yaml conf files, it will create a new var of type CommentedMap. This then fails on writing because CommentedMap does not provide the append function. CommentedSeq appears to be the correct data-type (and is what gets loaded in if the value does exist).

matt9j commented 3 years ago

Looks good! Could you wrap the commit message so it has a single top line with the high-level change, a new line, and then a wrapped body so it will format well in the log?

matt9j commented 3 years ago

Also, do you know if all instances where the old code used CommentedMap are correct with CommentedSeq? This commit changes the root function, and I don't know if other places were actually correctly using CommentedMaps (corresponding to YAML objects) vs CommengedSeq (which get turned into YAML arrays).

spencersevilla commented 3 years ago

That's a great question - I don't know for certain. It definitely is needed in the yaml context of an array (smf: subnet: - 10.45.0.1/16) but I don't know where else it's used. When I did some type checks, it looks to me like the yaml importer code is creating CommentedSeqs for these objects, so I figured this keeps us in line with what they're doing. Feel free to hold off merging if there are other places in the code you're worried about.

matt9j commented 3 years ago

Yeah, this is definitely not what we want to do. create_fields_helper is used by create_fields_if_not_exist, which is used for many types of fields, mostly ones that are actually maps/objects and not sequences/arrays. I think the function will need to be updated to be aware of the type it needs to be creating. I can attempt to do this...

spencersevilla commented 3 years ago

ok great - thanks for digging further! Also, this is far less urgent now that the other bug about yaml field names is fixed. This codepath should only arise when running colteconf over a open5gs conf file that's had some of the fieldnames explicitly removed from the default config.