search

paketo-buildpacks / libpak

An opinionated extension to the libcnb Cloud Native Buildpack Library
Apache License 2.0
15 stars 17 forks source link

Introduce struct with metadata to identify a dependency #267

Closed c0d1ngm0nk3y closed 12 months ago

c0d1ngm0nk3y commented 1 year ago

related to https://github.com/paketo-buildpacks/libpak/pull/233#issuecomment-1528865966

Summary

This introduces a Metadata struct with all relevant properties to identify a dependency and still be descriptive.

Use Cases

This allow users (e.g. libjvm) to use this metadata as layer metadata and not the whole dependency. That would a) Simplify comparison if there is a cache hit (special logic for deprecation_data) b) Ensure that no cache miss happens because some metadata changed (e.g. url)

Checklist

dmikusa commented 1 year ago

I'm not 100% sure I follow here. It looks like this PR is adding a method that will return a subset of the dependency's metadata for purposes of putting that into the layer contributor's metadata, which gets used for comparison/caching. If I'm understanding that correctly, it sounds good to me. I do have a couple of naming suggestions I'll add in different comments.

c0d1ngm0nk3y commented 1 year ago

I'm not 100% sure I follow here.

Fair enough. I had to leave, so my description felt a bit short. There are basically 3 steps:

1) Providing this structure to be able to filter the relevant metadata. First I wanted to go only for sha256, but then the metadata is not very descriptive. 2) Consume it in libjvm to set the metadata for a layer (e.g. here) 3) This allows to simplify the comparison of the metadata in libpak and get rid of this very specific logic. There should not be need for this anymore.

WDYT?

dmikusa commented 1 year ago

That sounds reasonable to me. A couple of clarifying questions...

First I wanted to go only for sha256, but then the metadata is not very descriptive.

Is the plan to still only compare on the sha256 hash? but to have this additional metadata for logging/output purposes?

This allows to simplify the comparison of the metadata in libpak and get rid of this very specific logic. There should not be need for this anymore.

Would that allow us to stop using reflect.DeepEqual for comparison?

This has been a source of confusion. When it returns false, it's never clear why and can sometimes. I'm guessing yes, since, going forward, we're just comparing the sha256 hashes.

c0d1ngm0nk3y commented 1 year ago

I have added the suggestions and once this is merged, released and consumed, libjvm can consume it.

c0d1ngm0nk3y commented 1 year ago

Is the plan to still only compare on the sha256 hash? but to have this additional metadata for logging/output purposes?

I think, I would keep using reflect.DeepEqual since the metadata could contain anything. True, ID, Name or Version could change with SHA256 being stable, but that sounds rather academic to me. Maybe in some odd edge cases, but probably we can live with that.

This allows to simplify the comparison of the metadata in libpak and get rid of this very specific logic. There should not be need for this anymore.

Would that allow us to stop using reflect.DeepEqual for comparison?

This has been a source of confusion. When it returns false, it's never clear why and can sometimes. I'm guessing yes, since, going forward, we're just comparing the sha256 hashes.

In this specific case, yes. But ExpectedMetadata is interface{} and could be anything, so I think reflect.DeepEqual is the right choice. But as long as ExpectedMetadata is only a simple and small struct, we should be fine. And this struct won't change if something got added to the Dependency over time.

c0d1ngm0nk3y commented 1 year ago

The usage would look like https://github.com/paketo-buildpacks/libjvm/pull/312.

After that, we don't need this coding (Equals) anymore.

Did I miss something?

c0d1ngm0nk3y commented 12 months ago

I don't fully understand the reasoning, but I will close this pr for v1 and #283 is open for v2.