Replace lib.TranslateKeys with a mapstructure decode hook #7963
Conversation
lib/decode/decode.go
Outdated
| @@ -0,0 +1,92 @@ | |||
| /*Package decode provides tools for customizing the decoding of configuration, | |||
There was a problem hiding this comment.
This might be the most nit picky comment ever but would this look better as either
/*
Package ...
*/
Or
// Package ...
// into ...
There was a problem hiding this comment.
Surprisingly golint doesn't like spaces before the package comment:
lib/decode/decode.go:1:1: package comment should not have leading space
However it seems to be happy with the following, so I will use that:
/*
Package decode provides tools for customizing the decoding of configuration
blogs, into structures using mapstructure.
*/
| return source, nil | ||
| } | ||
|
|
||
| // TODO: could be cached if it is too slow |
There was a problem hiding this comment.
There are performance issues with mapstructure especially when using decode hooks anways. This would be the least of our concerns.
| @@ -34,6 +34,8 @@ import ( | |||
| // // item's config field | |||
| // "widgets.config": "", | |||
| // }) | |||
| // | |||
| // Deprecated: Use lib/decode.HookTranslateKeys instead. | |||
There was a problem hiding this comment.
There is one place in the enterprise code thats still using this too. Otherwise I think this would be dead code and we could just get rid of it.
There was a problem hiding this comment.
I've left the CA provider use of this in OSS as well. I'm much less familiar with that piece of code so I wanted to split it out into a separate change. I expect we will be able to do the same thing.
This hook replaces lib.TranslateKeys and has a number of advantages: 1. Primarily, aliases for fields are defined on the field itself, making the aliases much easier to maintain, and more obvious to the reader. 2. TranslateKeys translation rules are not aware of structure. It could very easily incorrectly translate a key on one struct that was intended to be a translation rule for a completely different struct, leading to very hard to debug errors. The hook removes the need for the unexpected "translation rule is an empty string to indicate stop traversal" special case. 3. TranslateKeys attempts to duplicate a bunch of tree traversal logic that already exists in mapstructure. Using mapstructure for traversal removes the need to traverse the entire structure multiple times, and makes the behaviour more obvious to the reader. This change is being made to enable a future change of replacing PatchSliceOfMaps. TranslateKeys sits in between PatchSliceOfMaps and mapstructure.Decode, so it must be converted to a hook first, before PatchSliceOfMaps can be replaced by a decode hook.
With the exception of CA provider config, which will be migrated at some later time.
dnephin
force-pushed
the
dnephin/replace-lib-translate-keys
branch
from
db4260c
to
6a2d7d7
Compare
Replaces #7707 as the first of 3-4 PRs. In order to safely release the changes in #7935 we will need to first introduce forward compatibility. This PR is a pre-requisite to introducing that forward compatibility because it must happen between the normalization of HCL blocks and the decode.
This hook replaces lib.TranslateKeys and has a number of advantages:
the aliases much easier to maintain, and more obvious to the reader.
very easily incorrectly translate a key on one struct that was intended
to be a translation rule for a completely different struct, leading
to very hard to debug errors. The hook removes the need for the
unexpected "translation rule is an empty string to indicate stop
traversal" special case.
that already exists in mapstructure. Using mapstructure for traversal
removes the need to traverse the entire structure multiple times, and
makes the behaviour more obvious to the reader.
This change is being made to enable a future change of replacing
PatchSliceOfMaps. TranslateKeys sits in between PatchSliceOfMaps and
mapstructure.Decode, so it must be converted to a hook first, before
PatchSliceOfMaps can be replaced by a decode hook.