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 git buildpack. #39

Merged
merged 5 commits into from
Mar 11, 2021

Conversation

genevieve
Copy link

@genevieve genevieve commented Feb 8, 2021

Summary

Following paketo-buildpacks/feedback#4

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have added an integration test, if necessary.

@genevieve genevieve requested a review from a team as a code owner February 8, 2021 19:48
accepted/0022-git-buildpack.md Outdated Show resolved Hide resolved
accepted/0022-git-buildpack.md Outdated Show resolved Hide resolved
## Implementation

A new `git` buildpack will be developed to detect whether the `.git` directory, and then
read the directory to extract the following environment variables so that they
Copy link

@wburningham wburningham Feb 9, 2021

Choose a reason for hiding this comment

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

Could we also add an env var for the git branch and an env var for the git tag (if present)? My company injects those two items (plus the sha) into go binaries.

Choose a reason for hiding this comment

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

One more: knowing if a git repo is "dirty" with git status --porcelain would also be a helpful ENV var (for conditionally applying something based on whether or not there are uncommitted changes).

Copy link

@fg-j fg-j Feb 9, 2021

Choose a reason for hiding this comment

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

It's seeming likely that the set of env vars set by this buildpack would grow and grow with subsequent feature requests for users' specific needs. Is there a way to capture all of the information in the .git directory directly?

Copy link
Member

Choose a reason for hiding this comment

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

The contents of the concourse git resource might provide good prior art on exactly which data might be useful.

https://github.com/concourse/git-resource#additional-files-populated

@fg-j
Copy link

fg-j commented Feb 9, 2021

Readable

Copy link
Member

@ekcasey ekcasey left a comment

Choose a reason for hiding this comment

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

I really like this idea. The target use case makes sense and I think this buildpack could be extended to solve other problems in the future. For example, this buildpack could be extended to accept credentials from a binding and build time and configure git to pull from private repos, enabling the use of go modules from private repos.

## Proposal

Introduce an (optional) Git buildpack into each language buildpack family. The buildpack
will be charged with providing `git` metadata as build-time and run-time environment
Copy link
Member

Choose a reason for hiding this comment

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

I totally get why this is valuable at build time, but why is a runtime environment variable desirable?

Copy link
Member

Choose a reason for hiding this comment

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

I can imagine how providing this metadata through runtime environment variables might be useful for applications written in dynamic languages. Compiled languages might let you bake some of these values into the built executable, but unless we write these values down somewhere dynamic language applications won't have access to them at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

I am not as familiar with the typical patterns for doing things like this in a language like ruby or node(so let me know if I am way off base here), but I would have imagined that developers would want to template this into a static file or something like that a build time. The reason I would assume this: it seems like a static property of the build, not something that could/should be configured/injected at runtime.

I am instinctively wary of introducing coupling between the buildpack and the actual application code.

Introduce an (optional) Git buildpack into each language buildpack family. The buildpack
will be charged with providing `git` metadata as build-time and run-time environment
variables.

Copy link
Member

Choose a reason for hiding this comment

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

It would be really great if this buildpack could also contribute the following labels, defined in the OCI spec

  • org.opencontainers.image.source (public repos only?)
  • org.opencontainers.image.revision
  • org.opencontainers.image.version (maybe? if the commit is tagged?)

Currently the https://github.com/paketo-buildpacks/image-labels buildpacks allows users to set these explicitly but it would be a better experience for users if it just worked in cases where a .git directory was present.

We don't need to make this a condition of accepting the RFC but just wanted to suggest the idea while we are discussing this buildpack :)

accepted/0022-git-buildpack.md Outdated Show resolved Hide resolved
## Implementation

A new `git` buildpack will be developed to detect whether the `.git` directory, and then
read the directory to extract the following environment variables so that they
Copy link
Member

Choose a reason for hiding this comment

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

The contents of the concourse git resource might provide good prior art on exactly which data might be useful.

https://github.com/concourse/git-resource#additional-files-populated

@ryanmoran
Copy link
Member

I think the main point of an RFC like this is to justify that a buildpack deserves to exist. Based on the initial RFC and the comments asking for additional features, I believe this has met that requirement.

Rather than hashing out each of the possible features of this buildpack here, I'd like to see us adopt a buildpack with a small feature-set and then we can see RFCs or issues against the repo itself to discuss some of the finer details. Might I suggest that we begin with what is stated in the initial RFC, maybe with the addition of the current branch, if any.

I'd also like to see the discussion of the naming of these variables resolved. I agree with @ekcasey that choosing a name like REVISION instead of GIT_COMMIT_SHA might have more general benefit. As a compromise, you could implement both.

Finally, can we include what team would take over maintenance of this repo when it is implemented? It sounds like something that the existing Tooling team might support.

Co-authored-by: Ryan Moran <155736+ryanmoran@users.noreply.github.com>
@ryanmoran ryanmoran requested a review from ekcasey March 8, 2021 17:53
@ryanmoran ryanmoran merged commit abeafba into paketo-buildpacks:main Mar 11, 2021
@sambhav
Copy link
Contributor

sambhav commented Jun 1, 2021

Just pasting the notes from the WG meeting. There are some limitation to such a buildpack. Mainly, if the build is run in a sub-directory, the buildpack may not be able to see the .git dir during the build process. As a result, something lower level like a platform needs to implement this functionality.

There is an existing epic on pack side to support this buildpacks/pack#1139 which is actively being worked upon as an LFX project. kpack also already supports this out of the box.

The buildpack platform spec denotes that this data should also end up in the io.buildpacks.project.metadata label https://github.com/buildpacks/spec/blob/main/platform.md#iobuildpacksprojectmetadata-json

@ryanmoran
Copy link
Member

The intent of this buildpack goes beyond simply pulling metadata out of the codebase. We'd also like to use it to enable configuration of git itself such that it can be used to safely pull private repositories through package managers for a number of the supported language ecosystems.

After looking at the linked epic, I'm not sure how this buildpack would be superseded by the outlined work. Yes, there is some overlap that could be reduced with changes upstream, but considering that it does not seem to be an effort to enable git configuration or to enable access to this metadata by other buildpacks at buildtime, I still think this buildpack has a place.

modulo11 pushed a commit to sap-contributions/rfcs that referenced this pull request Oct 26, 2021
[paketo-buildpacks#33]

Signed-off-by: Ben Hale <bhale@vmware.com>
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.

6 participants