Support `BuildSpec` in `BuildRun` resources #1016

HeavyWombat · 2022-03-20T22:30:15Z

Changes

Fixes #677

Based upon SHIP #29

Core SHIP proposals:

Have BuildSpec in BuildRun
Embedded build spec does not lead to Build registered in the cluster.
Validation steps are done against embedded Build(Spec).
The embedded build spec, does not count as a Build in the context of the metrics package.
No TaskRun is created when Build(Spec) is invalid
BuildSpec is not used in combination with BuildRef, output, paramValues, env, or timeout.
README is updated with embedded example instead of Build and BuildRun example

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

Introducing support to embed a BuildSpec inside a BuildRun to have one-off builds, where only a BuildRun is required without the need of a Build resource. This includes an API change as the BuildRef in BuildRuns is no longer mandatory. Either BuildRef or BuildSpec can be used.

HeavyWombat · 2022-03-21T12:59:09Z

I only tested on Kube 1.21 so far, the E2E tests are failing in a 1.20 setup. Have to have a look at this.

SaschaSchwarze0 · 2022-03-21T13:02:25Z

I only tested on Kube 1.21 so far, the E2E tests are failing in a 1.20 setup. Have to have a look at this.

No worries @HeavyWombat, that looked like the typical hanging BuildAh build.

SaschaSchwarze0

Nice work. The pragmatic way of using an inline build has advantages and disadvantages. Without encouraging you to go away from it, there is one thing which imo should not happen: at certain places, we store a Build name: as a label on the BuildRun or TaskRun. And as label in Prometheus metrics. I don't think we should store a fake value there.

Imo BuildRun and TaskRun labels should not be set at all. In Prometheus, we must provide a value for the build label, but maybe "" is a better choice.

README.md

docs/buildrun.md

pkg/apis/build/v1alpha1/buildrun_types.go

pkg/reconciler/buildrun/buildrun.go

pkg/reconciler/buildrun/resources/build.go

pkg/reconciler/buildrun/buildrun.go

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

pkg/validate/validate.go

dalbar

Great work @HeavyWombat especially on the validation and tests. I have some minor change requests.

pkg/validate/validate.go

pkg/reconciler/buildrun/buildrun.go

pkg/reconciler/buildrun/resources/build.go

pkg/apis/build/v1alpha1/buildrun_types.go

qu1queee

Thanks this is nice. From my review I see the following:

[1] Introducing too many changes not related to this feature. Some of them are nice fixes, the rest I do not understand the rational, but they can go in. I'm just concerned that your code style is not gonna get widely adopted, so we will see what you change today again the future(take the imports as an example), not sure how much of value it have then. With this in mind, pls squash the following commits into one:

Fix potentially unused variable warning
Sort imports in build_test.go
Use specific description for bundle test
Add convenience checks for test env vars
Fix duplicate import
Refactor to fix potential variable shadow issue
Fix build not found test case
Clean-up Makefile
Fix declaration of "err" shadows declaration issue
Simplify if error case
Add/remove empty lines in buildrun_test.go

[2] I wrote a comment on this pr reintroducing a pointer. I was not involved on the discussion but we have that pointer before and then we went through a long discussion and that was removed. Wondering if you are aware of this.

qu1queee · 2022-03-22T19:43:23Z

pkg/apis/build/v1alpha1/buildrun_types.go

-	BuildRef BuildRef `json:"buildRef"`
+	//
+	// +optional
+	BuildRef *BuildRef `json:"buildRef"`


this is a regression of what was done recently in #397 , can u take a look?

No, this is fine. With the introduction of BuildSpec, the BuildRef needs to be optional now. Otherwise we could have both a build reference and an embedded build(spec).

pkg/reconciler/buildrun/buildrun.go

docs/buildrun.md

pkg/reconciler/build/build.go

HeavyWombat · 2022-03-22T21:24:49Z

Thanks this is nice. From my review I see the following:

[1] Introducing too many changes not related to this feature. Some of them are nice fixes, the rest I do not understand the rational, but they can go in. I'm just concerned that your code style is not gonna get widely adopted, so we will see what you change today again the future(take the imports as an example), not sure how much of value it have then. With this in mind, pls squash the following commits into one:
* Fix potentially unused variable warning

* Sort imports in build_test.go

* Use specific description for bundle test

* Add convenience checks for test env vars

* Fix duplicate import

* Refactor to fix potential variable shadow issue

* Fix build not found test case

* Clean-up Makefile

* Fix declaration of "err" shadows declaration issue

* Simplify if error case

* Add/remove empty lines in buildrun_test.go
[2] I wrote a comment on this pr reintroducing a pointer. I was not involved on the discussion but we have that pointer before and then we went through a long discussion and that was removed. Wondering if you are aware of this.

This work resulted in a number of related side changes. They added up over time to be honest. These were only done in files that I had to touch anyway. For consistency, I updated the same for example linter issue in the same file, too. Where possible, I opted to create a spin-off PR and moved the commit into it, i.e. #1018 and #1019.

With regards to the sorting order of imports, @gabemontero please keep me honest, I think we spoke about this in an earlier PR. The idea would be to have the Kubeish imports separately. I kind of like the idea, helps with getting a quick overview. However, we have no automated way to "enforce" the order. Some files have it, some do not. In come cases, other PRs updated the imports to have them sorted. If there is some easy and automated way to have that, we maybe should follow-up on that.

qu1queee · 2022-03-23T07:54:19Z

@HeavyWombat pls see the comment on squashing

pls squash the following commits into one

HeavyWombat · 2022-03-23T09:22:36Z

@HeavyWombat pls see the comment on squashing

pls squash the following commits into one

@qu1queee I fail to see the rational behind this to be honest. To squash them into one commit, I have to come up with a generic subject line for all of the combined changes. This will be very generic description. If I for example use GitLens, rather than a very specific subject, I only see the generic "fixing things" subject. Care to share your thoughts behind the squashing?

qu1queee

All good from my side, this is good stuff!!

gabemontero · 2022-03-31T17:03:37Z

Similar as in the other one, given this includes an API change, we'll need a second review. @adambkaplan @gabemontero

I'm good with API ... very similar pattern as to seen in k8s and tekton

I'll

/lgtm

but will

/hold

in case @adambkaplan gets time to take a look and chime in. If he doesn't by EOB Friday Apr 1, go ahead and unhold.

adambkaplan

Generally looks good, I request that you squash commits before merging (all code changes can be bundled together).

docs/buildrun.md

adambkaplan · 2022-03-29T17:59:54Z

docs/buildrun.md

+| False    | BuildRunNoRefOrSpec                     | Yes | BuildRun does not have either `BuildRef` or `BuildSpec` defined. There is no connection to a Build specification. |
+| False    | BuildRunAmbiguousBuild                  | Yes | The defined `BuildRun` uses both `BuildRef` and `BuildSpec`. Only one of them is allowed at the same time.|
+| False    | BuildRunBuildFieldOverrideForbidden     | Yes | The defined `BuildRun` uses an override (e.g. `timeout`, `paramValues`, `output`, or `env`) in combination with `BuildSpec`, which is not allowed. Use the `BuildSpec` to directly specify the respective value. |


Noteworthy that this is a good use case for a validating admission webhook (which we have previously discussed in #596)

Yeah, exactly, I dropped a comment with that in the code at the respective place.

adambkaplan · 2022-03-29T18:06:30Z

pkg/reconciler/buildrun/resources/build.go

+	if buildRun.Spec.BuildSpec != nil {
+		build.Name = ""
+		build.Namespace = buildRun.Namespace
+		build.Spec = *buildRun.Spec.BuildSpec


Perhaps a nit (I won't block this from merging). There's a bug risk here if somewhere downstream buildRun.Spec.BuildSpec is mutated. I would prefer a deep copy was used here.

Added a deepcopy instead. Thanks for the hint.

adambkaplan · 2022-03-29T18:08:52Z

pkg/reconciler/buildrun/resources/build.go

+		build.Name = ""
+		build.Namespace = buildRun.Namespace
+		build.Spec = *buildRun.Spec.BuildSpec
+		build.Status = buildv1alpha1.BuildStatus{}


Doesn't the Build object normally require certain status fields to be set in order for it to be used in a BuildRun? If so, it would make sense to set these here.

Yes, this is done in the reconciler code.

gabemontero · 2022-04-01T11:57:02Z

/unhold

Make `BuildRef` optional and generate CRDs and fix all occurrences. Introduce code in `GetBuildObject` to support both the `BuildRef` approach and the new `BuildSpec` option. Return a transient in-memory Build resource in case the BuildRun has an embedded BuildSpec. Add optional `BuildSpec` in BuildRun spec. Add E2E test cases for embedded builds in buildruns. Add new BuildRun validation option in docs. Remove variable name for `context` to make it undefined (linter warning). Add empty lines for readability. Sort imports based on common kube/others order. Replace copy/paste description with specific one for bundle tests. Add `Expect`s to give human readable error in case test environment variables are not set. Remove duplicate import of `k8s.io/api/core/v1`. Simplify code by removing the `if` check. Fix warning that variable declaration could shadow previous variable value by refactoring the respective `if` block. Add or remove empty lines in `buildrun_test.go` to increase readability and to keep the same style as in other test cases. Refactor code to fix variable value shadow warning. Replace generic `errors.New("not found")` with Kube specific one. Introduce empty status call stub (not not fail in status update case). Refactor return statement to be more simple. Introduce `BuildSpec` in BuildRun spec Refactor build reconciler logic by inlining `UpdateBuildStatusAndRetreat` function that is only used once. Add test case to cover special case for build resource not being in the system. Refactor validation code by moving out validation list into a global variable. Add convenience function to get the name of the referenced build. Use `resources.GetBuildObject` instead of manual look-up in test cases. Introduce controller-runtime Kubernetes client in test handle. Add functions to create the validation structs in a consistent way. Add convenience function to validate a Build with validation varargs. Introduce validation code for field permutations. Add test cases for field validation. Fix markdown linter warnings and indention.

HeavyWombat · 2022-04-01T12:08:13Z

Had a bit of a rebase hick-up of sorts. Should be fixed now.

SaschaSchwarze0 · 2022-04-04T09:33:00Z

Seeing all comments from Adam addressed, re-adding lgtm

/lgtm

qu1queee · 2022-04-04T09:34:58Z

/lgtm

HeavyWombat added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 20, 2022

openshift-ci bot added the release-note Label for when a PR has specified a release note label Mar 20, 2022

openshift-ci bot requested review from gabemontero and sbose78 March 20, 2022 22:30

HeavyWombat requested review from SaschaSchwarze0, qu1queee and dalbar March 20, 2022 22:34

HeavyWombat mentioned this pull request Mar 20, 2022

Update one-off build SHIP shipwright-io/community#69

Merged

qu1queee mentioned this pull request Mar 21, 2022

March 21st, 2022 Community Meeting shipwright-io/community#70

Closed

SaschaSchwarze0 requested changes Mar 21, 2022

View reviewed changes

dalbar suggested changes Mar 21, 2022

View reviewed changes

pkg/validate/validate.go Show resolved Hide resolved

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

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

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

HeavyWombat force-pushed the add/one-off-builds branch 2 times, most recently from 424731d to 7d1cf1d Compare March 22, 2022 13:56

qu1queee requested changes Mar 22, 2022

View reviewed changes

qu1queee reviewed Mar 22, 2022

View reviewed changes

pkg/reconciler/buildrun/buildrun.go Show resolved Hide resolved

qu1queee reviewed Mar 22, 2022

View reviewed changes

docs/buildrun.md Show resolved Hide resolved

qu1queee reviewed Mar 22, 2022

View reviewed changes

pkg/reconciler/build/build.go Show resolved Hide resolved

HeavyWombat force-pushed the add/one-off-builds branch from 7d1cf1d to 1eaa049 Compare March 22, 2022 21:10

HeavyWombat force-pushed the add/one-off-builds branch from 1eaa049 to 38139a0 Compare March 22, 2022 21:29

HeavyWombat force-pushed the add/one-off-builds branch 3 times, most recently from 0277f2a to 0523d53 Compare March 23, 2022 09:36

HeavyWombat requested review from qu1queee, dalbar and SaschaSchwarze0 March 23, 2022 09:41

qu1queee approved these changes Mar 30, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2022

openshift-ci bot assigned gabemontero Mar 31, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2022

adambkaplan reviewed Mar 31, 2022

View reviewed changes

HeavyWombat force-pushed the add/one-off-builds branch from c0b0f93 to 7aa3e37 Compare April 1, 2022 11:39

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 1, 2022

HeavyWombat force-pushed the add/one-off-builds branch from f0462e8 to 36dbd56 Compare April 1, 2022 11:45

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

HeavyWombat force-pushed the add/one-off-builds branch from 36dbd56 to b1ca5f2 Compare April 1, 2022 11:48

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

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2022

HeavyWombat force-pushed the add/one-off-builds branch from b1ca5f2 to ac18f62 Compare April 1, 2022 12:06

HeavyWombat requested review from adambkaplan, qu1queee and SaschaSchwarze0 April 1, 2022 12:07

SaschaSchwarze0 approved these changes Apr 1, 2022

View reviewed changes

openshift-ci bot assigned SaschaSchwarze0 Apr 4, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2022

openshift-ci bot assigned qu1queee Apr 4, 2022

openshift-merge-robot merged commit 5b03a5c into main Apr 4, 2022

qu1queee deleted the add/one-off-builds branch April 4, 2022 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `BuildSpec` in `BuildRun` resources #1016

Support `BuildSpec` in `BuildRun` resources #1016

HeavyWombat commented Mar 20, 2022 •

edited

Loading

HeavyWombat commented Mar 21, 2022

SaschaSchwarze0 commented Mar 21, 2022

SaschaSchwarze0 left a comment

dalbar left a comment

qu1queee left a comment

qu1queee Mar 22, 2022

HeavyWombat Mar 22, 2022

HeavyWombat commented Mar 22, 2022

qu1queee commented Mar 23, 2022

HeavyWombat commented Mar 23, 2022

qu1queee left a comment

gabemontero commented Mar 31, 2022

adambkaplan left a comment

adambkaplan Mar 29, 2022

HeavyWombat Apr 1, 2022

adambkaplan Mar 29, 2022

HeavyWombat Apr 1, 2022

adambkaplan Mar 29, 2022

HeavyWombat Apr 1, 2022

gabemontero commented Apr 1, 2022

HeavyWombat commented Apr 1, 2022

SaschaSchwarze0 commented Apr 4, 2022

qu1queee commented Apr 4, 2022

Support BuildSpec in BuildRun resources #1016

Support BuildSpec in BuildRun resources #1016

Conversation

HeavyWombat commented Mar 20, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

HeavyWombat commented Mar 21, 2022

SaschaSchwarze0 commented Mar 21, 2022

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

dalbar left a comment

Choose a reason for hiding this comment

qu1queee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeavyWombat commented Mar 22, 2022

qu1queee commented Mar 23, 2022

HeavyWombat commented Mar 23, 2022

qu1queee left a comment

Choose a reason for hiding this comment

gabemontero commented Mar 31, 2022

adambkaplan 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

gabemontero commented Apr 1, 2022

HeavyWombat commented Apr 1, 2022

SaschaSchwarze0 commented Apr 4, 2022

qu1queee commented Apr 4, 2022

Support `BuildSpec` in `BuildRun` resources #1016

Support `BuildSpec` in `BuildRun` resources #1016

HeavyWombat commented Mar 20, 2022 •

edited

Loading