Simplify buildpack.toml encoding/decoding
sophiewigmore opened this issue · 4 comments
The code in cargo/config.go is responsible for decoding buildpack.toml
files into Go structs, as well was encoding structs into TOML. This code is integral to the jam
tool.
There are a couple of concerns around this code, given how integral it is to our tools.
- The TOML decoding/encoding is done with the
BurntSushi/toml
package, which is in an unmaintained/archived status. It's worrisome to use a package in this state. - Due to the nature of the structs involved and the usage of the
BurntSushi/toml
package we have to convert into JSON as an intermittent step in order to decode/encode correctly. This makes for extremely complicated and hard to maintain code.
While all of this functionality works for our use case, this issue is here to document the technical debt associated with this code.
There is a newer TOML library, github.com/pelletier/go-toml that looks like a promising alternative for our use case. Upon initial investigation, there are a couple outstanding issues around using custom TOML marshalling that prevent us from using it currently. Upon the fix of theses issues this could be an option to resolve this.
@sophiewigmore Is this still blocked on improvements to pelletier/go-toml? If so, can you link to the outstanding issues? If not, feel free to remove the status/blocked
label.
Related to this issue, I notice that buildpack_info.go
and cargo/config.go
both contain structs that represent the contents of a buildpack.toml
. @paketo-buildpacks/tooling-maintainers , why do we have two separate representations? When the spec for buildpack.toml
contents changes, we end up having to make changes in two places, as with recent SBOM work.
@fg-j Interestingly enough, BurntSushi doesn't appear to be unmaintained or archived anymore, so this is less of a worrisome issue. However, the issue of complicated code still stands.
I've been digging around and I can't find the issues in go-toml I mentioned in the original issue (WHY didn't I link it then?) @ForestEckhardt we worked on this together, do you recall? Looking at releases for Go Toml, it seems like the new v2 release of the library has a lot of changes to the encoder/decoder. I wish I had been clearer in this issue about the actual problem we were seeing when we tried to switch over. For now, I wouldn't consider this blocked until we can determine that.
@fg-j RE: the need for buildpack_info.go
and cargo/config.go
my hunch is that we need Cargo's version to abstract away Paketo's opinionated choices in the structs from the top-level packit package. The fields in buildpack_info.go
are directly defined in the spec, so they can live in the top-level. It's still confusing, though. It might make more sense to pull in the top-level packit BuildpackInfo
struct to replace ConfigBuildpack
.