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

Add RFC for Stack metadata #78

Merged
merged 4 commits into from
Jul 29, 2020
Merged

Conversation

kvedurmu
Copy link
Contributor

@kvedurmu kvedurmu commented May 13, 2020

@kvedurmu kvedurmu requested a review from a team as a code owner May 13, 2020 15:11
Signed-off-by: Kashyap Vedurmudi <kvedurmudi@pivotal.io>
@hone
Copy link
Member

hone commented May 13, 2020

I assume this metadata is optional?

`io.buildpacks.stack.release`: Release Number
`io.buildpacks.stack.release_date`: Release date of image
`io.buildpacks.stack.description`: Description

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a generic metadata field that is defined by the stack author?

E.g., io.buildpacks.stack.metadata could be defined by another out-of-spec RFC when the stack ID is io.buildpacks.stacks.bionic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. I think that we should keep the other labels I proposed since we already use this pattern for the stack.id and stack.mixins and add this generic metadata label. How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on this. I quite like the pattern of top-level keys being wholly reserved for the spec (i.e. io.buildpacks.stack.*) and having a key that is wholly reserved for users that the spec will never use (io.buildpacks.stack.metadata). Top-level keys as they are described here look good.

Copy link
Member

Choose a reason for hiding this comment

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

E.g., io.buildpacks.stack.metadata could be defined by another out-of-spec RFC when the stack ID is io.buildpacks.stacks.bionic.

@sclevine can you elaborate on what you hope to have in there?

Copy link
Member

@ekcasey ekcasey May 20, 2020

Choose a reason for hiding this comment

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

I quite like the pattern of top-level keys being wholly reserved for the spec (i.e. io.buildpacks.stack.*) and having a key that is wholly reserved for users that the spec will never use (io.buildpacks.stack.metadata).

While I agree in theory, we currently use a lot of io.buildpacks.*.metadata for our own purposes, rather than reserving them for users. (e.g io.buildpacks.{lifecycle,build,builder}.metadata)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ekcasey Well that's a kick in the shorts. We can pick another key (.user) that represents the same thing, but alas is a naming inconsistency. My priority stack is as follows:

  1. Designated area for all users data that we both agree to never spec inside of and that a user will never create outside of
  2. Naming consistency

Even if that means we have some exceptions (e.g. io.buildpacks{lifecycle,build,builder}.metadata), I don't think we should introduce new inconsistencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kvedurmu I don't think this one has been addressed yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nebhale oops just updated it.

@kvedurmu
Copy link
Contributor Author

I assume this metadata is optional?
@hone yup, that's what I was thinking.

text/0000-stack-metadata.md Outdated Show resolved Hide resolved
text/0000-stack-metadata.md Outdated Show resolved Hide resolved
Signed-off-by: Kashyap Vedurmudi <kvedurmudi@pivotal.io>
`io.buildpacks.stack.release`: Release Number
`io.buildpacks.stack.release_date`: Release date of image
`io.buildpacks.stack.description`: Description

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on this. I quite like the pattern of top-level keys being wholly reserved for the spec (i.e. io.buildpacks.stack.*) and having a key that is wholly reserved for users that the spec will never use (io.buildpacks.stack.metadata). Top-level keys as they are described here look good.

text/0000-stack-metadata.md Show resolved Hide resolved
text/0000-stack-metadata.md Outdated Show resolved Hide resolved
text/0000-stack-metadata.md Show resolved Hide resolved
text/0000-stack-metadata.md Outdated Show resolved Hide resolved
@nebhale
Copy link
Contributor

nebhale commented Jun 17, 2020

@kvedurmu We'd like to get this moving again, can you please address the comments with new content?

@nebhale nebhale changed the base branch from master to main June 17, 2020 21:26
@kvedurmu
Copy link
Contributor Author

Apologies @nebhale lost track of this. I believe I've addressed all the comments and PRed in a spec change for this. Let me know if I'm missing anything.

Signed-off-by: Kashyap Vedurmudi <kvedurmudi@pivotal.io>
text/0000-stack-metadata.md Show resolved Hide resolved
# Drawbacks
[drawbacks]: #drawbacks

This could make image rebasing slightly more complicated as labels like stack version will need to be updated.
Copy link
Member

Choose a reason for hiding this comment

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

How would rebase work with this RFC?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the io.buildpacks.stack.* labels would need to be updated on the app image.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good to me. Can get how this works into the RFC itself?

@hone
Copy link
Member

hone commented Jul 1, 2020

I'm overall 👍 this RFC, but would like to see the rebase case addressed that was listed.

text/0000-stack-metadata.md Outdated Show resolved Hide resolved
@hone
Copy link
Member

hone commented Jul 15, 2020

We'll need to fix the DCO on my suggested commit. :(

@nebhale
Copy link
Contributor

nebhale commented Jul 15, 2020

Final Comment Period with merge disposition, closing on 22 July, 2020.

@nebhale
Copy link
Contributor

nebhale commented Jul 15, 2020

@kvedurmu Please do a rebase where you sign off on all the commits.

Co-authored-by: Terence Lee <hone02@gmail.com>
Signed-off-by: Kashyap Vedurmudi <kvedurmudi@pivotal.io>
nebhale added a commit that referenced this pull request Jul 29, 2020
[#78]

Signed-off-by: Ben Hale <bhale@vmware.com>
@nebhale nebhale merged commit 53bf13b into buildpacks:main Jul 29, 2020
@kvedurmu kvedurmu deleted the stack-metadata branch July 29, 2020 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants