Params EP Implementation #781

qu1queee · 2021-05-17T20:58:53Z

Changes

Implements the parameterize-strategies EP

Note: I modified the API, as spec.params in all CRD´s might be confusing I decided to use:

spec.params for Build and BuildRuns
spec.parameters for Strategies.

I would open another PR after this one gets merged, to introduce a tutorial on how to build a strategy with parameters.

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

Release Notes

Add support for parameters in the strategies, so that users can parameterize their usage.

SaschaSchwarze0

Nice work. I have so many use cases in mind to leverage that. :-)

Based on our usual agreement, as it contains API changes, requires a second review. @imjasonh having some minutes?

/approve
/assign imjasonh

openshift-ci · 2021-06-14T15:03:17Z

[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

imjasonh

Mostly lgtm, some small readability comments but otherwise nothing structural.

Thanks for this PR, and sorry it took so long for me to review it.

pkg/reconciler/buildrun/resources/params_test.go

imjasonh · 2021-06-17T13:57:53Z

pkg/reconciler/buildrun/resources/params.go

+	}
+
+	// override params that matches by name in both originalParams and overrideParams
+	for i, o := range originalParams {


It might be a bit more readable and performant to first convert both lists into map[string]Param, then you can do lookups without having to double-loop.

I don't think in practice the performance will be noticeable, but I get nervous any time I see nested fors.

yes, good idea, done.

pkg/reconciler/buildrun/resources/params_test.go

imjasonh · 2021-06-17T14:05:21Z

pkg/reconciler/buildrun/resources/taskrun.go

@@ -37,6 +38,10 @@ const (
 	inputParamContextDir = "CONTEXT_DIR"
 )

+var (
+	systemReservedParametersRegexp = regexp.MustCompile("BUILDER_IMAGE|DOCKERFILE|CONTEXT_DIR|(shp-.*)")


Could this instead be expressed as a helper function that checks if the string is in a set of disallowed strings, and doesn't have the shp- prefix?

I find that regular expressions can be hard to read and understand, which can lead to bugs. If it can be implemented in reasonably brief code that's usually better in my experience:

var reserved := map[string]bool{ "BUILDER_IMAGE": true, "DOCKERFILE": true, "CONTEXT_DIR": true, } func isReserved(n string) bool { return reserved[n] || strings.HasPrefix("shp-") }

yes, nice, done.

pkg/validate/strategies.go

qu1queee

@imjasonh added all your request for changes, thanks

imjasonh · 2021-06-24T13:56:57Z

pkg/apis/build/v1alpha1/parameter.go

-type Parameter struct {
+// Param is a key/value that populates a strategy parameter
+// used in the execution of the strategy steps
+type Param struct {


How do you feel about ParamValue, a field named paramValues:, etc.? It's a bit confusing to have both Parameter and Param types (Tekton has this issue as well)

not my favorite field name tbh, but I see the benefit. The concern I have now, is that these naming conventions were already accepted via the related EP before, by different people.

My options on this:

Block this till consensus from more folks, as the change params->paramValues is high.

Merge this, and then look for consensus. If we need to change this, then we PR the change.

What do you think?

@imjasonh I did the changes as requested. FYI I sync on this offline with @SaschaSchwarze0 and @HeavyWombat , we all agree its a good alternative. Please re-review this PR again.

pkg/validate/strategies.go

Extend Build and BuildRun to support spec.params Extend the Strategies to support spec.parameters Regenerate CRD´s files

Support test cases for using params. Apply this to both namespaced and cluster-scope strategies.

Ensure that on TaskRun creation we can add the strategy parameters as part of the Task parameters, and that we populate their values via the Build/BuildRun params definition.

Validate that a param is: - not colliding with system parameters - exists in the strategy parameters - overrides via a BuildRun can happen, as long as the param exists in the Build Also extend Build custom errors, to support two new ones: - RestrictedParametersInUse - UndefinedParameter

- Extend Build docs - Extend BuildRun docs - Extend Strategy docs - Update parameterize EP and set to `implemented`.

Make a clear distinction on calling the Build/BuildRun params as `spec.paramValues` instead of `spec.params`

imjasonh

/lgtm

Thanks!

qu1queee added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021

openshift-ci bot added the release-note Label for when a PR has specified a release note label May 17, 2021

openshift-ci bot requested review from gabemontero and imjasonh May 17, 2021 20:58

qu1queee removed request for imjasonh and gabemontero May 17, 2021 20:59

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

qu1queee force-pushed the qu1queee/ep_params_implementation branch from 4c01830 to 1928515 Compare May 23, 2021 19:46

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

qu1queee force-pushed the qu1queee/ep_params_implementation branch 10 times, most recently from 15bdb71 to 7852d9f Compare May 30, 2021 19:07

qu1queee force-pushed the qu1queee/ep_params_implementation branch 4 times, most recently from 410baa6 to eb8209c Compare May 31, 2021 08:04

qu1queee removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2021

qu1queee changed the title ~~WIP: Params EP Implementation~~ Params EP Implementation May 31, 2021

qu1queee requested review from HeavyWombat, imjasonh and SaschaSchwarze0 and removed request for HeavyWombat May 31, 2021 08:10

qu1queee force-pushed the qu1queee/ep_params_implementation branch 3 times, most recently from d509e90 to 4e696a3 Compare June 14, 2021 14:17

qu1queee requested a review from SaschaSchwarze0 June 14, 2021 14:17

qu1queee force-pushed the qu1queee/ep_params_implementation branch from 4e696a3 to 2294c13 Compare June 14, 2021 15:00

SaschaSchwarze0 approved these changes Jun 14, 2021

View reviewed changes

openshift-ci bot assigned imjasonh Jun 14, 2021

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

imjasonh reviewed Jun 17, 2021

View reviewed changes

qu1queee force-pushed the qu1queee/ep_params_implementation branch 2 times, most recently from 5b46715 to f014205 Compare June 22, 2021 18:48

qu1queee requested a review from imjasonh June 22, 2021 18:51

qu1queee commented Jun 22, 2021

View reviewed changes

qu1queee force-pushed the qu1queee/ep_params_implementation branch 2 times, most recently from 90956bf to a1e0909 Compare June 23, 2021 10:21

imjasonh reviewed Jun 24, 2021

View reviewed changes

qu1queee force-pushed the qu1queee/ep_params_implementation branch from a1e0909 to 3a9cdf1 Compare June 24, 2021 14:35

qu1queee requested a review from imjasonh June 24, 2021 14:44

qu1queee added 6 commits June 25, 2021 12:29

Add API changes

f7ed176

Extend Build and BuildRun to support spec.params Extend the Strategies to support spec.parameters Regenerate CRD´s files

Extend integration tests

53b0dc4

Support test cases for using params. Apply this to both namespaced and cluster-scope strategies.

Add params business logic

9863c25

Ensure that on TaskRun creation we can add the strategy parameters as part of the Task parameters, and that we populate their values via the Build/BuildRun params definition.

Update docs

1fb74b7

- Extend Build docs - Extend BuildRun docs - Extend Strategy docs - Update parameterize EP and set to `implemented`.

Modify Parameterize EP

1970780

Make a clear distinction on calling the Build/BuildRun params as `spec.paramValues` instead of `spec.params`

qu1queee force-pushed the qu1queee/ep_params_implementation branch from 3a9cdf1 to 1970780 Compare June 25, 2021 10:33

imjasonh reviewed Jun 25, 2021

View reviewed changes

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

openshift-merge-robot merged commit 43ed730 into shipwright-io:main Jun 25, 2021

qu1queee deleted the qu1queee/ep_params_implementation branch June 25, 2021 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Params EP Implementation #781

Params EP Implementation #781

qu1queee commented May 17, 2021 •

edited

Loading

SaschaSchwarze0 left a comment

openshift-ci bot commented Jun 14, 2021

imjasonh left a comment

imjasonh Jun 17, 2021

qu1queee Jun 22, 2021

imjasonh Jun 17, 2021

qu1queee Jun 22, 2021

qu1queee left a comment

imjasonh Jun 24, 2021

qu1queee Jun 24, 2021 •

edited

Loading

qu1queee Jun 25, 2021

imjasonh left a comment

Params EP Implementation #781

Params EP Implementation #781

Conversation

qu1queee commented May 17, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 14, 2021

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh Jun 17, 2021

Choose a reason for hiding this comment

qu1queee Jun 22, 2021

Choose a reason for hiding this comment

imjasonh Jun 17, 2021

Choose a reason for hiding this comment

qu1queee Jun 22, 2021

Choose a reason for hiding this comment

qu1queee left a comment

Choose a reason for hiding this comment

imjasonh Jun 24, 2021

Choose a reason for hiding this comment

qu1queee Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

qu1queee Jun 25, 2021

Choose a reason for hiding this comment

imjasonh left a comment

Choose a reason for hiding this comment

qu1queee commented May 17, 2021 •

edited

Loading

qu1queee Jun 24, 2021 •

edited

Loading