ensure all the structs include proper yaml labels #69
Conversation
fgiudici
changed the title
fgiudici
force-pushed
the
struct_yaml_labels
branch
from
7d545c1 to
6eef8eb
Compare
fgiudici
changed the title
Codecov Report
@@ Coverage Diff @@
## main #69 +/- ##
=======================================
Coverage 29.74% 29.74%
=======================================
Files 5 5
Lines 353 353
=======================================
Hits 105 105
Misses 244 244
Partials 4 4
Continue to review full report at Codecov.
|
| NoSMBIOS bool `json:"noSMBIOS,omitempty"` | ||
| Labels map[string]string `json:"labels,omitempty"` | ||
| URL string `json:"url,omitempty" yaml:"url,omitempty" mapstructure:"url"` | ||
| CACert string `json:"ca-cert,omitempty" yaml:"ca-cert,omitempty" mapstructure:"ca-cert"` |
There was a problem hiding this comment.
understanding we needed a mapstructure tag here has been tricky! 😅
There was a problem hiding this comment.
OMG! At which stage do we convert a struct to a map!? All this way more convoluted that it should, there should only be a single procedure to serialize and another one to deserialize 😅
I am wondering why do we only need it for these two, it is weird that only two fields of the struct are dumped to or loaded from a map, while other not.
There was a problem hiding this comment.
the mapstructure package is used by viper to do the MergeInConfig here, which processes the yaml file we put in the iso (which basically has only url and ca-cert values).
Only the url and the ca-cert are needed at that point to connect to the operator (cluster side). Once there we get the full yaml config, from which we do our "standard" yaml deserialization without passing through viper MergeInConfig 😅
There was a problem hiding this comment.
Oh yes, it's true, viper it's based on mapstructure, this why we have mapstructure and yaml labels in elemental-cli.
Good catch! Thanks for the insights
| NoSMBIOS bool `json:"noSMBIOS,omitempty"` | ||
| Labels map[string]string `json:"labels,omitempty"` | ||
| URL string `json:"url,omitempty" yaml:"url,omitempty" mapstructure:"url"` | ||
| CACert string `json:"ca-cert,omitempty" yaml:"ca-cert,omitempty" mapstructure:"ca-cert"` |
There was a problem hiding this comment.
OMG! At which stage do we convert a struct to a map!? All this way more convoluted that it should, there should only be a single procedure to serialize and another one to deserialize 😅
I am wondering why do we only need it for these two, it is weird that only two fields of the struct are dumped to or loaded from a map, while other not.
note that we need also to add the 'mapstructure' tag to the CACert field to allow proper yaml decode from viper: viper takes care of merging the configuration from the yaml file and uses mapstructure for decoding the yaml file. The only fields merged by viper are the URL and the CACert. Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
fgiudici
force-pushed
the
struct_yaml_labels
branch
from
6eef8eb to
fba0f86
Compare
we need also to add the 'mapstructure' tag to the CACert field to allow proper yaml decode from viper: viper takes care of
merging the configuration from the yaml file and uses mapstructure for decoding the yaml file.
The only fields merged by viper are the URL and the CACert, so added the mapstructure tags only to those.
Part of #51