Add per-field suppression to extraconfig encoder#8038
Conversation
zjs
left a comment
zjs
left a comment
There was a problem hiding this comment.
If it turns out that need need
Small typo in the commit message.
| svm := simulator.Map.Get(ref).(*simulator.VirtualMachine) | ||
|
|
||
| template := config.VirtualContainerHostConfigSpec{} | ||
| keys := extraconfig.CalculateKeys(template, "ExecutorConfig.ExecutorConfigCommon.ID", "") |
There was a problem hiding this comment.
Unrelated to this PR, but: It seems like it would be helpful to have a extraconfig.CalculateKey helper that calls extraconfig.CalculateKeys, verifies that a single key is returned, and returns the single value to the caller. This would avoid the need to follow the extraconfig.CalculateKeys call with an unchecked [0] all over the place.
| const ( | ||
| // DefaultTagName value | ||
| // GuestInfoPrefix is dictated by vSphere | ||
| GuestInfoPrefix = "guestinfo." |
There was a problem hiding this comment.
Unrelated to this change, but it seems like this prefix is hard-coded elsewhere:
vic/pkg/vsphere/extraconfig/secret.go
Line 30 in 1387ab1
vic/lib/portlayer/exec/base.go
Lines 147 to 151 in 1387ab1
vic/lib/migration/manager/manager.go
Lines 35 to 36 in 1387ab1
Should those be updated to refer to this constant?
There was a problem hiding this comment.
The manager should be updated with a test that asserts that the key returned by extraconfig.CalculateKey matches the embedded string so we're alerted if we ever try to change the version location.
For the secrets, yes.
For base.go - this should be using CalculateKey
| RecurseTag = "recurse" | ||
|
|
||
| // HiddenScope means the key is hidden from the guest and will not have a GuestInfoPrefix | ||
| HiddenScope = "hidden" |
There was a problem hiding this comment.
Should any of the instances of this string be updated to use this constant?
vic/pkg/vsphere/extraconfig/keys_test.go
Line 38 in 1387ab1
vic/pkg/vsphere/extraconfig/keys_test.go
Line 42 in 1387ab1
vic/pkg/vsphere/extraconfig/keys_test.go
Line 50 in 1387ab1
| // HiddenScope means the key is hidden from the guest and will not have a GuestInfoPrefix | ||
| HiddenScope = "hidden" | ||
| // ReadOnlyScope means the key is read-only from the guest | ||
| ReadOnlyScope = "read-only" |
There was a problem hiding this comment.
As above, keys-test.go contains hard-coded instances of this string (and others, below).
This adds a recursion property to skip encode or decode of a field. If skipping both the depth=0 should be used. This also extractst most string constants used in the package and turns them into actual constants. This also splits DefaultGuestInfoPrefix which should have only contained the vSphere mandatory guestinfo prefix but also contained the default VIC "vice" namespace prefix. As part of this it move those constants that should be modifiable to be variables. Future work should extend this to be a defined config structure that Encode and Decode can operate from.
This updates the extraconfig package to use the constants introduced by the earlier commit in the PR. This also updates extraconfig to have a CalculateKey function in addition to the CalculateKeys function that returns an array. This has been done because of an observation that by far the most common usage was CalculateKeys(...)[0], sometimes with length checking of the return, frequently without. The new method panics if the structure pattern for the field cannot be found.
hickeng
force-pushed
the
write-only-extraconfig
branch
from
1c9a96b to
4405d0e
Compare
Adds a recursion property to skip encode or decode of a field. If skipping both the depth=0 should be used. Splits DefaultGuestInfoPrefix. It should have only contained the vSphere mandatory guestinfo prefix but also contained the default VIC "vice" namespace prefix. As part of this it extracts most string constants used in the package and turns them into actual constants, and moves those constants that should be modifiable to be variables. Future work should extend this to be a defined config structure that Encode and Decode can operate from. Updates extraconfig to have a CalculateKey function in addition to the CalculateKeys function that returns an array. This has been done because of an observation that by far the most common usage was CalculateKeys(...)[0], sometimes with length checking of the return but frequently without. The new method panics if the structure pattern for the field cannot be found. (cherry picked from commit 525fbcf)
…e#8101) Adds a recursion property to skip encode or decode of a field. If skipping both the depth=0 should be used. Splits DefaultGuestInfoPrefix. It should have only contained the vSphere mandatory guestinfo prefix but also contained the default VIC "vice" namespace prefix. As part of this it extracts most string constants used in the package and turns them into actual constants, and moves those constants that should be modifiable to be variables. Future work should extend this to be a defined config structure that Encode and Decode can operate from. Updates extraconfig to have a CalculateKey function in addition to the CalculateKeys function that returns an array. This has been done because of an observation that by far the most common usage was CalculateKeys(...)[0], sometimes with length checking of the return but frequently without. The new method panics if the structure pattern for the field cannot be found. (cherry picked from commit 525fbcf)
Adds a recursion property to skip encode or decode of a field. If skipping both the depth=0 should be used. Splits DefaultGuestInfoPrefix. It should have only contained the vSphere mandatory guestinfo prefix but also contained the default VIC "vice" namespace prefix. As part of this it extracts most string constants used in the package and turns them into actual constants, and moves those constants that should be modifiable to be variables. Future work should extend this to be a defined config structure that Encode and Decode can operate from. Updates extraconfig to have a CalculateKey function in addition to the CalculateKeys function that returns an array. This has been done because of an observation that by far the most common usage was CalculateKeys(...)[0], sometimes with length checking of the return but frequently without. The new method panics if the structure pattern for the field cannot be found. (cherry picked from commit 525fbcf)
4 participants
Turns the annotation string labels for controlling serialization behaviour
into constants with comments. Still requires package level descripton
and examples.
Adds a recursion property to skip encode or decode of a field. If skipping
both the depth=0 should be used.
Splits DefaultGuestInfoPrefix into pieces. The vSphere mandatory guestinfo
prefix should not be user configurations, whereas the key "namespacing"
prefix should have been. For VIC this was the "vice" namespace prefix which
was not applied to the hidden (non-guestinfo) keys. As such this commit
drops the DefaultPrefix from the encode/decode paths for now due to
upgrade constraints.
There is now an explicit separation between those elements that should not
be modified (constants) and those that should be usage configuration (vars).
Future work should extend this with a defined config structure that Encode
and Decode can operate from.
If it turns out that we need
initialize oncerather thanskip-decodefor anyfield, we can look at making the field a pointer and having explicit logic based
around whether the target is a nil pointer or not. This gives us easy inline
tracking of whether initialization has occurred, although with unfortunate
dependence on the user not having explicitly initialized some pointer paths in
the target structure ahead of time.
Towards #7410