Introduce struct with metadata to identify a dependency #267

c0d1ngm0nk3y · 2023-07-07T15:05:54Z

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

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

dmikusa · 2023-07-07T16:09:32Z

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.

buildpack.go

c0d1ngm0nk3y · 2023-07-07T17:34:46Z

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:

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.
Consume it in libjvm to set the metadata for a layer (e.g. here)
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 · 2023-07-07T17:45:44Z

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.

Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>

Co-authored-by: Daniel Mikusa <dan@mikusa.com>

c0d1ngm0nk3y · 2023-07-28T14:36:47Z

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

dmikusa · 2023-07-28T18:33:05Z

layer.go

@@ -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(),


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?

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.

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.

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?

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.

c0d1ngm0nk3y · 2023-08-01T06:26:05Z

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 · 2023-08-01T06:53:19Z

The usage would look like paketo-buildpacks/libjvm#312.

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

Did I miss something?

c0d1ngm0nk3y · 2023-09-15T08:20:17Z

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

c0d1ngm0nk3y requested a review from a team as a code owner July 7, 2023 15:05

c0d1ngm0nk3y force-pushed the dep-metadata branch from 5b631e4 to b2209d6 Compare July 7, 2023 15:06

dmikusa added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Jul 7, 2023

dmikusa reviewed Jul 7, 2023

View reviewed changes

buildpack.go Outdated Show resolved Hide resolved

dmikusa reviewed Jul 7, 2023

View reviewed changes

buildpack.go Outdated Show resolved Hide resolved

dmikusa reviewed Jul 7, 2023

View reviewed changes

buildpack.go Outdated Show resolved Hide resolved

c0d1ngm0nk3y and others added 4 commits July 28, 2023 16:33

Introduce struct with metadata to identify a dependency

7e951cd

Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>

Update buildpack.go

53a2629

Co-authored-by: Daniel Mikusa <dan@mikusa.com>

Update buildpack.go

33159cf

Co-authored-by: Daniel Mikusa <dan@mikusa.com>

Update buildpack.go

bcb3de4

Co-authored-by: Daniel Mikusa <dan@mikusa.com>

c0d1ngm0nk3y force-pushed the dep-metadata branch from 890c89a to bcb3de4 Compare July 28, 2023 14:42

dmikusa reviewed Jul 28, 2023

View reviewed changes

Merge branch 'main' into dep-metadata

5c6fa4f

c0d1ngm0nk3y mentioned this pull request Aug 1, 2023

Use new metadata struct for dependency metadata paketo-buildpacks/libjvm#312

Draft

5 tasks

c0d1ngm0nk3y mentioned this pull request Aug 18, 2023

Introduce struct with metadata to identify a dependency #283

Merged

5 tasks

c0d1ngm0nk3y closed this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce struct with metadata to identify a dependency #267

Introduce struct with metadata to identify a dependency #267

c0d1ngm0nk3y commented Jul 7, 2023

dmikusa commented Jul 7, 2023

c0d1ngm0nk3y commented Jul 7, 2023

dmikusa commented Jul 7, 2023

c0d1ngm0nk3y commented Jul 28, 2023

dmikusa Jul 28, 2023

c0d1ngm0nk3y Aug 4, 2023

dmikusa Aug 4, 2023

c0d1ngm0nk3y Aug 18, 2023

dmikusa Aug 18, 2023

c0d1ngm0nk3y commented Aug 1, 2023

c0d1ngm0nk3y commented Aug 1, 2023

c0d1ngm0nk3y commented Sep 15, 2023

Introduce struct with metadata to identify a dependency #267

Introduce struct with metadata to identify a dependency #267

Conversation

c0d1ngm0nk3y commented Jul 7, 2023

Summary

Use Cases

Checklist

dmikusa commented Jul 7, 2023

c0d1ngm0nk3y commented Jul 7, 2023

dmikusa commented Jul 7, 2023

c0d1ngm0nk3y commented Jul 28, 2023

dmikusa Jul 28, 2023

Choose a reason for hiding this comment

c0d1ngm0nk3y Aug 4, 2023

Choose a reason for hiding this comment

dmikusa Aug 4, 2023

Choose a reason for hiding this comment

c0d1ngm0nk3y Aug 18, 2023

Choose a reason for hiding this comment

dmikusa Aug 18, 2023

Choose a reason for hiding this comment

c0d1ngm0nk3y commented Aug 1, 2023

c0d1ngm0nk3y commented Aug 1, 2023

c0d1ngm0nk3y commented Sep 15, 2023