-
Notifications
You must be signed in to change notification settings - Fork 17
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
Introduce struct with metadata to identify a dependency #267
Closed
c0d1ngm0nk3y
wants to merge
5
commits into
paketo-buildpacks:main
from
sap-contributions:dep-metadata
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7e951cd
Introduce struct with metadata to identify a dependency
c0d1ngm0nk3y 53a2629
Update buildpack.go
c0d1ngm0nk3y 33159cf
Update buildpack.go
c0d1ngm0nk3y bcb3de4
Update buildpack.go
c0d1ngm0nk3y 5c6fa4f
Merge branch 'main' into dep-metadata
c0d1ngm0nk3y File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExpectedMetadata on the DependencyLayerContributor is an interface{} so I'm not sure I follow how adding a type here is going to help. We're going to immediately throw away that type.
Should we be changing ExpectedMetadata to have a type? but that could be considered a breaking change because ExpectedMetadata is a public field, so I'm not sure we can change the type.
Am I missing something here? Are you just going to cast this in libjvm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but assigning it to
interface{}
should be fine, shouldn't it? The type information is not really important since we would still usereflect.DeepEqual
. But there are only 4 strings in this struct to compare. See paketo-buildpacks/libjvm#312 on how the change inlibjvm
would look like.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just struggling with the risk/benefit on this one. I get that we're only comparing a subset of metadata, and so that should make the comparison work better, but on the other hand we've already fixed the comparison in a different way. It's not exactly an elegant solution, but it works. I'm just hesitant to change the code again, which always introduces some risk as well as maintenance burden pushing releases through everywhere.
Especially since I can say with 99.99% certainty that I would not approve a PR against v1 that does change the metadata structure at this point. Anything like that should go into v2.
I wouldn't be opposed to doing something like this in v2. I think we can definitely do better in this regard. I'd love to get rid of some of the
interface{}
and casting and of course the deep equals and replace it with generics. Go generics is an option now that we're on 1.20 as a project. I think we have the possibility of introducing some generics from the ground up in libcnb and libpak which might make this all cleaner.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, I don't see any risk with that one. I kind of see your point, but I think it is important to keep the current version in a good shape and not push everything to the next shiny release. But I see that this might be tempting. But it is your call.
Then I will open a pr for
v2
instead. But this will mean that the cleanup step will take quite a while because it will take some time untilv2
is consumed.BTW, is there a ETA for
v2
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the libcnb v2 repo is in pretty good shape. It's possible things could still change, but it's only likely to change with regards to support for the next buildpack API & removing stacks (0.10).
As such, we've been working on v2 for libpak. We're tagging v2 alpha releases so it's easy for folks to start testing them. There's still a high probability for code change, so I wouldn't start porting buildpacks unless you want to be part of the effort in designing and evolving the libpak API, then it's a perfect time to get involved.
It all kind of goes hand-in-hand, but we're doing the same with pipeline-builder & libjvm. We've not started libbs yet.