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

Simplify buildpack.toml encoding/decoding #179

Open
sophiewigmore opened this issue May 13, 2021 · 4 comments
Open

Simplify buildpack.toml encoding/decoding #179

sophiewigmore opened this issue May 13, 2021 · 4 comments
Labels
enhancement A new feature or request status/blocked This issue has been triaged and resolving it is blocked on some other issue

Comments

@sophiewigmore
Copy link
Member

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.

  1. 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.
  2. 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 sophiewigmore added the enhancement A new feature or request label May 13, 2021
@fg-j
Copy link

fg-j commented Nov 30, 2021

@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.

@fg-j
Copy link

fg-j commented Nov 30, 2021

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 fg-j added the status/blocked This issue has been triaged and resolving it is blocked on some other issue label Nov 30, 2021
@sophiewigmore
Copy link
Member Author

@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.

@sophiewigmore
Copy link
Member Author

sophiewigmore commented Dec 3, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or request status/blocked This issue has been triaged and resolving it is blocked on some other issue
Projects
None yet
Development

No branches or pull requests

2 participants