Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

fixes #658 #659

Merged
merged 1 commit into from Mar 14, 2019
Merged

fixes #658 #659

merged 1 commit into from Mar 14, 2019

Conversation

sbawaska
Copy link
Contributor

enable Unmarshalling of "Images" in duffle.json

enable Unmarshalling of "Images" in duffle.json
@@ -82,14 +82,14 @@ type ImagePlatform struct {

// Image describes a container image in the bundle
type Image struct {
BaseImage
BaseImage `mapstructure:",squash"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch here!
I do have two questions regarding this change:

  • any particular reason you didn't include the json tag as well?
  • we haven't used squash for fields so far -- could you, please, include a couple of sentences why we need to add it, and why it's inconsistent with all other fields in the bundle?

Thanks!

Copy link
Contributor Author

@sbawaska sbawaska Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason you didn't include the json tag as well?

using squash causes the other structure to basically be inlined, as a result the json tag from the BaseImage struct will be used.

we haven't used squash for fields so far -- could you, please, include a couple of sentences why we need to add it, and why it's inconsistent with all other fields in the bundle?

As described in the bug #658, not having the squash tag prevents the Image from marshalling the fields of the BaseImage struct.

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually tested, and everything seems to be in order.
I do have one question about the tags used, but after that is sorted, LGTM.

Thanks for your contribution!

@technosophos
Copy link
Member

Somehow I missed this PR, so it should merge before #662 and I will rebase that one afterward

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks for your contribution!

@radu-matei radu-matei merged commit adcaa3a into cnabio:master Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants