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

Introduce struct with metadata to identify a dependency #267

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions buildpack.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,31 @@ type BuildpackDependency struct {
DeprecationDate time.Time `toml:"deprecation_date"`
}

// DependencyLayerContributorMetadata returns the subset of data from BuildpackDependency that is use as expected metadata for the DependencyLayerContributor.
type DependencyLayerContributorMetadata struct {
// ID is the dependency ID.
ID string `toml:"id"`

// Name is the dependency name.
Name string `toml:"name"`

// Version is the dependency version.
Version string `toml:"version"`

// SHA256 is the hash of the dependency.
SHA256 string `toml:"sha256"`
}

// GetMetadata return the relevant metadata of this dependency
func (b BuildpackDependency) GetMetadata() DependencyLayerContributorMetadata {
return DependencyLayerContributorMetadata{
ID: b.ID,
Name: b.Name,
Version: b.Version,
SHA256: b.SHA256,
}
}

// Equals compares the 2 structs if they are equal. This is very simiar to reflect.DeepEqual
// except that properties that will not work (e.g. DeprecationDate) are ignored.
func (b1 BuildpackDependency) Equals(b2 BuildpackDependency) bool {
Expand Down
2 changes: 1 addition & 1 deletion layer.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ func NewDependencyLayer(dependency BuildpackDependency, cache DependencyCache, t
func NewDependencyLayerContributor(dependency BuildpackDependency, cache DependencyCache, types libcnb.LayerTypes) DependencyLayerContributor {
return DependencyLayerContributor{
Dependency: dependency,
ExpectedMetadata: dependency,
ExpectedMetadata: dependency.GetMetadata(),
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you just going to cast this in libjvm?

No, but assigning it to interface{} should be fine, shouldn't it? The type information is not really important since we would still use reflect.DeepEqual. But there are only 4 strings in this struct to compare. See paketo-buildpacks/libjvm#312 on how the change in libjvm would look like.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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.

I wouldn't be opposed to doing something like this in v2.

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 until v2 is consumed.

BTW, is there a ETA for v2?

Copy link
Contributor

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.

DependencyCache: cache,
ExpectedTypes: types,
}
Expand Down