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 support for docker URI images in buildpackstore. #301

Closed
wants to merge 2 commits into from

Conversation

robdimsdale
Copy link
Member

Summary / Use Cases

This PR adds support for docker/OCI images in the buildpackstore. This allows us to use pre-built images as dependent buildpacks instead of having to build them from scratch each time.

Using OCI images as dependencies has multiple advantages:

  1. It is more correct, as we're testing against the actual buildpacks that have been created, rather than building from scratch.
  2. Extending the above, point, it is trivially-easy to pin the dependent buildpacks to known versions. Currently that is quite difficult (potentially impossible) in occam/freezer.
  3. It is more efficient and stable (as we can leverage the docker cache and not worry about accidentally corrupting the freezer cache)

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

@robdimsdale robdimsdale requested a review from a team as a code owner May 20, 2024 21:42
@robdimsdale robdimsdale added the semver:major A change requiring a major version bump label May 20, 2024
@robdimsdale robdimsdale marked this pull request as draft May 20, 2024 21:43
@robdimsdale robdimsdale force-pushed the docker-uri-images-buildpackstore branch from c828385 to 5fc6e35 Compare May 20, 2024 21:49

offline bool
version string
}

func (g BuildpackStoreGet) Execute(url string) (string, error) {
if strings.HasPrefix(url, "docker://") {
if g.oci == nil {
return "", fmt.Errorf("must provide OCI fetcher to fetch OCI images")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is kind of gross, but it's the only way I could see to make this backwards-compatible and avoid a breaking change in the exported function signatures.

The OCI fetcher has to be injected, so either we force the user to inject it at construction time (i.e. NewBuildpackStore(dockerPull BPStoreDockerPull)) which is a breaking change, or we accept that it might be nil when we try to use it.

@robdimsdale robdimsdale marked this pull request as ready for review May 20, 2024 21:52
@robdimsdale robdimsdale added semver:minor A change requiring a minor version bump and removed semver:major A change requiring a major version bump labels May 20, 2024
@robdimsdale
Copy link
Member Author

After chatting with @ForestEckhardt I think we can close this, and instead require buildpack authors to explicitly switch to occam.Docker.Pull in the integration tests when they switch to a docker image in the integration config.

This abstraction is probably unnecessary.

@robdimsdale robdimsdale deleted the docker-uri-images-buildpackstore branch May 21, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant