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

Use packit.BuildpackInfo inside cargo.Config #261

Closed
wants to merge 1 commit into from
Closed

Conversation

sophiewigmore
Copy link
Member

@sophiewigmore sophiewigmore commented Dec 3, 2021

Summary

In #179 (comment), @fg-j asks why we have duplication across packit.BuildpackInfo and cargo.ConfigBuildpack.

I also can't see why we need to have both of these structs, so I have removed cargo.ConfigBuildpack and modified packit.BuildpackInfo to contain the appropriate json labels.

If there is a reason for the duplication, I would like an explanation so we have that historical context in place going forward.

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@sophiewigmore sophiewigmore requested a review from a team as a code owner December 3, 2021 20:51
@ryanmoran
Copy link
Member

I think I'm fine with this in theory, but I don't like that we ended up with changes to the top-level packit library (the addition of the JSON tags) that only need to be there to support some internal code that lives in cargo. I think this would make a lot more sense after we resolve the code complexity in cargo that forces us to convert to JSON and then back to TOML.

@sophiewigmore
Copy link
Member Author

@ryanmoran I hear that! I wasn't sure if the JSON fields would really matter much but I think that's a valid concern. Let me draft this PR until I've had a chance to look at go-toml v2

@sophiewigmore sophiewigmore marked this pull request as draft December 3, 2021 22:25
@ryanmoran ryanmoran changed the base branch from main to v2 December 7, 2021 16:11
@sophiewigmore
Copy link
Member Author

sophiewigmore commented Dec 21, 2021

I investigated using the go-toml v2 beta release to skip the JSON conversion step, but I couldn't get it into a working state. Closing this PR out until we can figure out a solid way to encode/decode the cargo.Config type.

@ryanmoran ryanmoran deleted the cargo-config branch January 23, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants