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.

Sign up for GitHub

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

Jump to bottom

RFC for OCI Image Annotations on Buildpacks #314

Merged
merged 7 commits into from
Aug 8, 2024
Merged

RFC for OCI Image Annotations on Buildpacks #314

merged 7 commits into from
Aug 8, 2024

Conversation

candrews
Copy link
Contributor

The buildpack tool should set OCI annotations on the OCI artifacts it produces providing users of these buildpacks with a consistent, standard mechanism for gathering information about the buildpack, including how to find its documentation and what version it is. The annotation values can be gathered from existing data sources (such as buildpack.toml) therefore not requiring any additional effort on the part of users of the buildpack tool.

Originally proposed to Paketo at paketo-buildpacks/rfcs#307 where it was pointed out that this change would best be made in the buildpack tool, see paketo-buildpacks/rfcs#307 (comment)

@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

@candrews candrews mentioned this pull request Jun 26, 2024
Closed
5 tasks
edmorley
edmorley reviewed Jun 26, 2024
View reviewed changes
text/0000-oci-image-annotations.md

When building the buildpack, the `buidpack` tool can get the values for the `org.opencontainers.image.source` and `org.opencontainers.image.revision` annotations from git. `org.opencontainers.image.source` is derived from the git origin and `org.opencontainers.image.revision` is the git commit ref.

The other annotation values come from `buildpack.toml` mapped to OCI annotations as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when an app has more than one buildpack? The proposed schema doesn't support more than one entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RFC is for the buildpacks themselves - not for the apps built by the buildpacks.

For example, it would apply to the Paketo Java buildpack, gcr.io/paketo-buildpacks/java, not to the application it builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. I think the RFC wording might need adjusting to make that clearer.

For example here:

The buildpack tool should set OCI annotations on the OCI artifacts it produces

"buildpack tool" isn't a phrase that's really used by this project, and read to me as meaning "the buildpack".

So it then read as:

"The buildpack should set OCI annotations on the OCI artifacts it produces"

...and the only OCI artifacts created when the buildpack runs is the app OCI image.

It sounds like what you're really proposing is a change to the buildpack distribution specification:
https://github.com/buildpacks/spec/blob/main/distribution.md

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like what you're really proposing is a change to the buildpack distribution specification:

Combined with proposing that the Pack CLI implement these changes, so that when a buildpack maintainer runs pack buildpack package to package their buildpack into the OCI image form of distribution it has the proposed labels set.

Copy link
Contributor Author

@candrews candrews Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of:

The buildpack tool should set...

I've updated the RFC to say:

pack buildpack package should set

Does that eliminate the confusion?

It sounds like what you're really proposing is a change to the buildpack distribution specification:
https://github.com/buildpacks/spec/blob/main/distribution.md

I believe so - is there something I should do in this RFC to indicate as much?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that eliminate the confusion?

Yeah that does thank you.

I believe so - is there something I should do in this RFC to indicate as much?

So I think this comes down to whether this RFC is proposing only a change to the Pack CLI's own implementation, or a change to the specification which then the Pack CLI (plus any other implementations) would implement. If the latter, than the spec change could be described under the "spec change" section of the RFC template.

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've updated this MR to indicate that the Distribution spec would also be updated.

Copy link

Choose a reason for hiding this comment

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

I'm still seeing a lot of references to: "buildpack tool". Can we ensure this RFC is consistent in its terminology?

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've updated all occurrences of "buildpack tool" to be "pack tool" for consistency

@candrews candrews force-pushed the oci-image-annotations branch 3 times, most recently from 3147747 to a9a4863 Compare June 26, 2024 20:43
@natalieparellano
Copy link
Member

natalieparellano commented Jun 27, 2024 •
edited
Loading

Awesome to see this! I believe this supersedes #196

Edit: I am wrong, this is about buildpacks themselves, not the artifacts they produce!

@natalieparellano natalieparellano mentioned this pull request Jul 1, 2024
Open
@jkutner jkutner self-assigned this Jul 11, 2024
@shresthaujjwal
Copy link

Thanks for opening this RFC. May I recommend extending this RFC not just to buildpack but also to builders?

@candrews
Copy link
Contributor Author

Thanks for opening this RFC. May I recommend extending this RFC not just to buildpack but also to builders?

That sounds like a good idea to me!

@jkutner
Copy link
Member

jkutner commented Jul 18, 2024

This is looking good. I'm going to bring it up in working group today, but then I think it's ready to move forward. Thanks @candrews

@jkutner
Copy link
Member

jkutner commented Jul 28, 2024

FCP should have ended last Thursday, but I failed to note that FCP had started. I'm going to give this until next week, and then finalize it. Sorry for the delay.

@jkutner
Copy link
Member

jkutner commented Aug 8, 2024

This RFC is accepted. I'm just trying to figure out the merge process (our tooling is broken :( )

jkutner added a commit that referenced this pull request Aug 8, 2024
@jkutner
RFC 0130
4c05331 
[#314]

Signed-off-by: Joe Kutner <[email protected]>
@jkutner jkutner merged commit c2c92ab into buildpacks:main Aug 8, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robdimsdale robdimsdale robdimsdale left review comments

@joshwlewis joshwlewis joshwlewis left review comments

@edmorley edmorley edmorley left review comments

@jkutner jkutner jkutner approved these changes

Assignees

@jkutner jkutner

Labels
audience/buildpack-author Final Comment Period status/voting team/distribution type/rfc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@candrews @buildpack-bot @natalieparellano @shresthaujjwal @jkutner @edmorley @joshwlewis @robdimsdale @jabrown85