Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add struct tag to Buildpack.Path field #86

Merged
merged 3 commits into from Nov 4, 2021
Merged

Add struct tag to Buildpack.Path field #86

merged 3 commits into from Nov 4, 2021

Conversation

jghiloni
Copy link
Contributor

@jghiloni jghiloni commented Nov 4, 2021

Most Go applications use github.com/burntsushi/toml as their toml parsing library. When struct tags are omitted, the library will do case-insensitive deserialization, so a property called path will successfully unmarshal into the Path struct field.

However, if youmarshal a Buildpack instance to toml, the Path field appears regardless of whether it is set or not, and technically violates the buildpack toml spec. This PR adds a struct tag to not only marshal the field to the path property in the resultant toml, but also adds the omitempty modifier to ignore the field altogether when not set.

This resolves #85

Signed-off-by: Josh Ghiloni jghiloni@vmware.com

Most Go applications use github.com/burntsushi/toml as their toml
parsing library. When struct tags are omitted, the library will do
case-insensitive deserialization, so a property called "path" will
successfully unmarshal into the Path struct field. However, if you
marshal a Buildpack instance to toml, the "Path = " field appears
regardless of whether it is set or not, and technically violates the
buildpack toml spec. This PR adds a struct tag to not only marshal the
field to the "path" property in the resultant toml, but also adds the
"omitempty" modifier to ignore the field altogether when not set.

Signed-off-by: Josh Ghiloni <jghiloni@vmware.com>
@dmikusa
Copy link
Contributor

dmikusa commented Nov 4, 2021

I don't believe that path is part of the spec for buildpack.toml. My understanding is that libcnb adds this field so an author knows where the buildpack resides. I think we should just omit that field altogether if we're reading or writing a buildpack.toml.

@dmikusa dmikusa added semver:patch A change requiring a patch version bump type:bug A general bug labels Nov 4, 2021
@jghiloni
Copy link
Contributor Author

jghiloni commented Nov 4, 2021

That sounds good -- I'll add another commit changing that behavior

Signed-off-by: Josh Ghiloni <jghiloni@vmware.com>
buildpack_test.go Outdated Show resolved Hide resolved
Signed-off-by: Josh Ghiloni <jghiloni@vmware.com>
@samj1912 samj1912 merged commit 8c10662 into buildpacks:main Nov 4, 2021
@jghiloni jghiloni deleted the issue-85 branch November 4, 2021 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buildpack struct's Path field missing struct tag
3 participants