Local source code feature (using bundle images) #835

HeavyWombat · 2021-07-13T13:19:48Z

Changes

Reference #717

Add new bundle download step container, analog to Git step.

Update Build spec to add optional Container field.

Add bundle/unbundle functions for the newly added bundle images, where Bundle
packs a local directory as-is into a one layer container image and Unbundle
does the reverse by unpacking the layers of an input image into a target
directory. This includes the registry push and pull operations, respectively.

Update TaskRun setup code to either add a Git step or bundle step.

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Introduced support for so called bundle images, which contain packaged source code and can be used instead of a Git repository for builds.

SaschaSchwarze0

I just checked the ship, I thought @qu1queee wanted a source result for the digest of the image, but that has not made it to the EP. I think would be source-${sourceName}-image-digest. But can also be separately added.

pruneBundle is in the API but not implemented. It is fine with me if that gets implemented in a second step, but then it should also not be in the API.

cmd/bundle/main.go

deploy/crds/shipwright.io_builds.yaml

pkg/reconciler/buildrun/resources/sources/bundle.go

pkg/reconciler/buildrun/resources/sources/bundle_core.go

SaschaSchwarze0 · 2021-07-26T16:34:37Z

samples/build/build_buildpacks-v3-bundle.yaml

@@ -0,0 +1,16 @@
+---


Should this be under the test/data directory as this is not necessarily a good fit for our samples directory? Same applies to other files that you added.

I figured those could be samples for reference. However, given that they differ only slightly from the Git base counterparts, I have no problem at all to remove them.

test/e2e/validators_test.go

SaschaSchwarze0

A few more small things.

cmd/bundle/main.go

pkg/apis/build/v1alpha1/source.go

pkg/reconciler/buildrun/resources/sources.go

HeavyWombat · 2021-08-04T11:32:49Z

I just checked the ship, I thought @qu1queee wanted a source result for the digest of the image, but that has not made it to the EP. I think would be source-${sourceName}-image-digest. But can also be separately added.

pruneBundle is in the API but not implemented. It is fine with me if that gets implemented in a second step, but then it should also not be in the API.

The prune functionality was completely moved into the CLI code. I will make some adjustments to the EP.

SaschaSchwarze0

/approve

openshift-ci · 2021-08-05T14:41:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [SaschaSchwarze0]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gabemontero

Just some minor items @HeavyWombat

looks good !

gabemontero · 2021-08-11T17:23:41Z

pkg/reconciler/buildrun/resources/sources/bundle_core.go

@@ -241,6 +241,7 @@ func unpack(in io.Reader, targetPath string) error {
 			}

 			if _, err := io.Copy(file, tr); err != nil {
+				file.Close()


I think it is better to add a defer file.Close() immediately after the if err != nil ... block that precedes the io.Copy call

that way, if later changes get inserted in between these lines, we don't lose the call to close

I searched for an alternative to this, too, but since it is in a for loop, I think there is no other way to write it. The only other option would be to externalize the loop block into a separate function, but I figured that would be overkill.

yeah I've done the externalize in a for loop before .... I'd be inclined to do that / in my opinion at least do not consider it overkill

gabemontero · 2021-08-11T17:28:58Z

pkg/apis/build/v1alpha1/source.go

+	//
+	// +optional
+	Container *Container `json:"container"`
+


a confirmation / agreement comment here @HeavyWombat @SaschaSchwarze0 @otaviof ... having pointer ref here is conducive to allowing for multiple "implementation" choices for the upload

i.e. we add a new pointer that enables use of a kubectl cp equivalent

so cool deal

we probably will just need to make sure when we get to that point that there is validation that only allows one of these optional fields related to upload mechanism to be set

Good point. I have not thought about it much, I guess I kind of was under the impression that every "type" of input would eventually end up having their respective individual entry here, therefore making it a pointer reference. Depending on how many other "types" we want to introduce, we could end up with a lot of optional fields, but I guess this is ok.

yeah we have had as many a 4 "options" in some places in openshift ... so yes "seems OK"

bottom line, we are all good here :-) .... occasionally it is just good to provide positive feedback in PRs as well :-)

gabemontero · 2021-08-11T17:30:49Z

pkg/apis/build/v1alpha1/source.go

@@ -8,11 +8,24 @@ import (
 	corev1 "k8s.io/api/core/v1"
 )

+// Container describes the source code bundle container to pull
+type Container struct {


I do wonder if Container as the name of a struct type is overloaded, given its use in core k8s

how opposed would everyone be to a more precise name like ImageBundleContainer or BundleImageContainer ?

Or BundleContainer as I see a BundleConatinerTemplate field below

gabemontero · 2021-08-11T17:39:15Z

pkg/reconciler/buildrun/resources/sources/bundle_core.go

+	var tw = tar.NewWriter(&buf)
+	defer tw.Close()
+
+	err := filepath.Walk(directory, func(path string, info os.FileInfo, err error) error {


According to the doc at https://pkg.go.dev/path/filepath#Walk the newer WalkDir is more efficient.

Though that is new with 1.16, and we still are at 1.15 per https://github.com/shipwright-io/build/blob/main/.github/workflows/ci.yml

I'm fine if we don't want to take on moving to 1.16 for the project in this PR, but if that is the path we take, how about a TODO comment here to switch to WalkDir when we do move to 1.16

I think we are fine to move to 1.16 as 1.17 is out there. It was a nice exercise to change it to WalkDir. Although, I think the improvements will not be that good for us as we require the d.Info() call on almost all files anyway.

Add quote to make sure empty build names can be detected quickly. Fix `%w` with `%v` as the `%w` is only for `fmt.Errorf`.

With the removal of `Ginkgo` as the default test driver in favour for `go test`, the output becomes quite verbose and more difficult to quickly check. Remove log line from test case to make test output less verbose.

Remove unused variable assignment.

Reference #717 Add new bundle download step container, analog to Git step. Update Build spec to add optional Container field. Add bundle/unbundle functions for the newly added bundle images, where `Bundle` packs a local directory as-is into a one layer container image and `Unbundle` does the reverse by unpacking the layers of an input image into a target directory. This includes the registry push and pull operations, respectively. Update TaskRun setup code to either add a Git step or bundle step.

github.com/docker/cli to `v20.10.7`

Apply suggestions from code review. Co-authored-by: Sascha Schwarze <[email protected]>

Bump Go version to 1.16 to be able to use `WalkDir`. Switch to `WalkDir` function.

gabemontero

/lgtm

not blocking on my one remaining comment item, but noting it in the PR in case it ever comes up when some looks at this in the future

gabemontero · 2021-08-30T18:53:02Z

pkg/bundle/bundle.go

+				return err
+			}
+
+			file, err := os.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode))


I won't hold the PR up for this, but will cite that per our previous conversation, that I think the os.OpenFile through the io.Copy should be in a separate method, so you can employ defer file.Close in that separate method, and not get the golang complaint about using defer's directly in a for loop.

openshift-ci bot added release-note Label for when a PR has specified a release note and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 13, 2021

openshift-ci bot requested review from adambkaplan and qu1queee July 13, 2021 13:19

HeavyWombat requested review from otaviof and SaschaSchwarze0 July 13, 2021 13:20

HeavyWombat added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 13, 2021

HeavyWombat mentioned this pull request Jul 16, 2021

Local source code feature (using bundle images) shipwright-io/cli#35

Closed

4 tasks

HeavyWombat removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2021

SaschaSchwarze0 requested changes Jul 26, 2021

View reviewed changes

SaschaSchwarze0 requested changes Jul 29, 2021

View reviewed changes

cmd/bundle/main.go Show resolved Hide resolved

pkg/apis/build/v1alpha1/source.go Outdated Show resolved Hide resolved

pkg/reconciler/buildrun/resources/sources.go Outdated Show resolved Hide resolved

HeavyWombat requested a review from SaschaSchwarze0 August 4, 2021 11:33

SaschaSchwarze0 reviewed Aug 5, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2021

SaschaSchwarze0 approved these changes Aug 5, 2021

View reviewed changes

gabemontero suggested changes Aug 11, 2021

View reviewed changes

SaschaSchwarze0 mentioned this pull request Aug 16, 2021

August 16th, 2021 Community Meeting shipwright-io/community#22

Closed

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2021

HeavyWombat added 3 commits August 30, 2021 14:21

Update log statements in e2e tests

1064230

Add quote to make sure empty build names can be detected quickly. Fix `%w` with `%v` as the `%w` is only for `fmt.Errorf`.

Remove log line from test case

3376735

With the removal of `Ginkgo` as the default test driver in favour for `go test`, the output becomes quite verbose and more difficult to quickly check. Remove log line from test case to make test output less verbose.

Fix this value of tr is never used (SA4006)

0aefd78

Remove unused variable assignment.

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2021

HeavyWombat requested review from gabemontero and SaschaSchwarze0 August 30, 2021 16:11

HeavyWombat removed the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Aug 30, 2021

HeavyWombat and others added 4 commits August 30, 2021 18:15

bump(*):

e23d471

github.com/docker/cli to `v20.10.7`

Apply suggestions from code review

d3b548f

Apply suggestions from code review. Co-authored-by: Sascha Schwarze <[email protected]>

Switch to WalkDir in bundle package

5f6b547

Bump Go version to 1.16 to be able to use `WalkDir`. Switch to `WalkDir` function.

gabemontero reviewed Aug 30, 2021

View reviewed changes

openshift-ci bot assigned gabemontero Aug 30, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2021

openshift-merge-robot merged commit 04ff81c into shipwright-io:main Aug 30, 2021

HeavyWombat deleted the add/local-source branch September 1, 2021 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local source code feature (using bundle images) #835

Local source code feature (using bundle images) #835

HeavyWombat commented Jul 13, 2021

SaschaSchwarze0 left a comment

SaschaSchwarze0 Jul 26, 2021

HeavyWombat Jul 29, 2021

SaschaSchwarze0 left a comment

HeavyWombat commented Aug 4, 2021

SaschaSchwarze0 left a comment

openshift-ci bot commented Aug 5, 2021

gabemontero left a comment

gabemontero Aug 11, 2021

HeavyWombat Aug 30, 2021

gabemontero Aug 30, 2021

gabemontero Aug 11, 2021

HeavyWombat Aug 30, 2021

gabemontero Aug 30, 2021

gabemontero Aug 11, 2021

gabemontero Aug 11, 2021

gabemontero Aug 11, 2021

HeavyWombat Aug 30, 2021

gabemontero left a comment

gabemontero Aug 30, 2021

Local source code feature (using bundle images) #835

Local source code feature (using bundle images) #835

Conversation

HeavyWombat commented Jul 13, 2021

Changes

Submitter Checklist

Release Notes

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

HeavyWombat commented Aug 4, 2021

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 5, 2021

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment