Merge the same TOML file writing logic#1822
Conversation
github-actions
Bot
added
the
type/enhancement
ESWZY
force-pushed
the
merge-toml-writing
branch
2 times, most recently
from
1b236e8 to
36c234e
Compare
joe-kimmel-vmw
left a comment
joe-kimmel-vmw
left a comment
There was a problem hiding this comment.
Thanks for contributing @ESWZY !
It looks like your intention is to "DRY" up this part of the code by merging redundant codepaths? I think that's a fine idea overall though I'll defer to the actual pack maintainers' judgement.
I've made a couple of suggestions. I think as a project we really do want our error messages to be as helpful and informative as possible, so maintaining the existing specificity of the error messages is probably desired.
| buf := &bytes.Buffer{} | ||
| err := toml.NewEncoder(buf).Encode(stack) | ||
| if err != nil { | ||
| return errors.Wrap(err, "marshaling stack metadata") |
There was a problem hiding this comment.
imo the loss of specific error messages here and above is undesirable, but we could fix this by passing in one additional parameter to writeToml something like dataDescriptor string and it could then be given the value stack metadata on line 253 below, and appropriate values elsewhere. What do you think?
|
|
||
| return ctrClient.CopyToContainer(ctx, containerID, "/", reader, types.CopyToContainerOptions{}) | ||
| // WriteToml writes a `data.toml` for test only. | ||
| func WriteToml(dstPath string, data interface{}, os string) ContainerOperation { |
There was a problem hiding this comment.
I think rather than introduce a new public API function only for the tests, it would be better to test one of the existing public functions
|
Thanks for your generous suggestions, I will consider it! |
Signed-off-by: Woa <me@wuzy.cn>
joe-kimmel-vmw
left a comment
joe-kimmel-vmw
left a comment
There was a problem hiding this comment.
LGTM but it'll have to wait for a pack maintainer review :)
5 participants
Summary
Seeing that there is the same TOML file writing logic here, for maintainability considerations, they can be merged together.
Output
No change.
Documentation