SHIP 0025 Extended parameter support

[SaschaSchwarze0]

Changes

This pull requests implements the extended parameter support from ship 0025 BuildStrategy Parameter Enhancements. It also updates the BuildKit sample build strategy to (a) use a simple way to behave like the other Dockerfile strategies wrt context-dir and dockerfile, and uses the new array parameters feature.

The YAML API has been extended in a backward-compatible way as designed in the ship. The changes to the Go types are not backward compatible.

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

Action required: The parameter support in build strategy now includes arrays. Build users can start to reference parameter values from ConfigMaps and Secrets. If you previously used the go types with parameters, then you will need to make slight changes to adopt to the changed type structure.

[gabemontero]

just making sure @SaschaSchwarze0 ... with the WIP label still in place your intent is we should wait before reviewing this EP. Is that correct?

thanks

[SaschaSchwarze0]

just making sure @SaschaSchwarze0 ... with the WIP label still in place your intent is we should wait before reviewing this EP. Is that correct?

thanks

Correct, though, if you want, go ahead and review the code. With the validations that I recently added, the code should be more or less complete. I have to fill some test gaps.

[gabemontero]

Hey @SaschaSchwarze0 - given time constraints for me my review at least will have to be in stages. To that, your nice organization of the changes in the various commits is facilitating that. Thank you for that :-)

So I'm starting with the API commit. Just have one item for our discussion. Though admittedly it may engender some debate amongst us. Maybe not though - let's find out.

[gabemontero]

On Fri, Jan 21, 2022 at 12:30 PM Sascha Schwarze ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/build/v1alpha1/parameter.go <#975 (comment)>: > // ParamValue is a key/value that populates a strategy parameter // used in the execution of the strategy steps type ParamValue struct { - Name string `json:"name"` - Value string `json:"value"` Hi @gabemontero <https://github.com/gabemontero>, the value field is not going away in the serialized (yaml/json) format. In ParamValue, I am inlining SingleValue which brings back the value field. So, we are here breaking "only" the Go API (like we also do in Shahul's PR where we fix the handling of optional fields). But the "YAML API" is not broken.

Ahhh … totally glossed over / missed the inline element and use of “value” in SingleValue So yeah nevermind 😀 Will move on to next commit later today or Monday The alternative of not inlining is to have configMapValue, secretValue, and

…

value copied into ParamValue. That would be okay for me as well. But, I would still need to break the Go API. Previously value was not optional and no pointer. Now it is both because it is just one of the three possible fields that can be set to define a value (and in case of an array, none of them would be set). — Reply to this email directly, view it on GitHub <#975 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3NU5DP4LZ2UTYCKFIZ5X3UXGJZ5ANCNFSM5LMYIAIQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[gabemontero]

Just some minor suggestions for the document changes.

[gabemontero]

an impressive amount of test coverage @SaschaSchwarze0

only 1 minor possible addition came to my mind as I was trying to digest all this ... let me know what you think

[gabemontero]

some minor suggestions / questions on the implementation commit @SaschaSchwarze0

[gabemontero]

interesting approach ... borrowed from somewhere, or an original from you?

I suspect it in part stems from the restrictions / constratins on env var names. Perhaps a comment detailing the highlights of that would be good.

WDYT?

[SaschaSchwarze0]

What exactly surprised you?

I defined in the ship that the environment variable gets a random suffix. And because tools like gosec complain every random number usage that is not using crypto/rand, I used crypto/rand. Unfortunately that beast only has a signature to work with big.Ints rather than primitive ints, which makes the code look a little more complex than needed, especially at the top when EnvVarNameSuffixCharsCount is initialized. If you're aware of a simpler solution, let me know.

I admin that I actually did not check which restrictions on environment variable names exist. I just used uppercase letters and digits for simplicity.

BTW: the purpose of the random suffix is not to obfuscate something, it is rather that I need an environment variable name that is unique.

[gabemontero]

yeah "surprise" is the wrong choice of words for my sentiment here... its connotation is not what I was trying to convey.

let me try again

the algorithm was novel and new to me; that is not at all a bad thing; in fact, there is novelty and inventiveness in it that I'm trying to convey; I was more curious, because of the novelty, if you derived it entirely by yourself, or if you were inspired from somewhere else in the k8s ecosystem, that is all

hopefully addressing that piece, even if the env var name restrictions did not come into play for you, I think it is an aspect which your algorithm achieves, and could be part of a comment you add. re: restrictions, see https://github.com/kubernetes/api/blob/107b3100d94652e198a0e866ed0c2d028e6649f0/core/v1/types.go#L197 and https://github.com/kubernetes/kubernetes/blob/f2ddd60eb9e7e9e29f7a105a9a8fa020042e8e52/pkg/api/v1/types.go#L42-L4

some of your history of what was attempted previously could also be useful to new users who view the code in the future without knowing immediately to look for say the SHIP or something

anyway, if you are amenable to adding some comments, great; I won't block the PR if you feel strongly the code is self explanatory.

[gabemontero]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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 [gabemontero]

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

[gabemontero]

ok @SaschaSchwarze0 I think other than a yay/nay on adding comments on your neat algorithm, around its current approach and dismissed alternatives, based on my clarification in #975 (comment) and it hopefully better indicating my intent, I've submitted all the review I'll be able to on this one.

So I'll proactive

/lgtm

with the note that if you do add that comment, I'll quickly re-lgtm if need be.

I'll also, for now,

/hold

in that admittedly my review was a bit distracted because of getting this in with a bunch of other multi-tasking I've had to do. Ideally, others in the community manage to take a pass at this. Possibly catch things I have missed, though my high level sense is you've nailed this. However, we also want to get this into v0.8.0. So if it turns out no one else can get time for it before our v0.8.0 cut off, go ahead and cancel the hold, and we'll go with just my review.

[SaschaSchwarze0]

Thanks @gabemontero, please wait some more minutes. I started to add comments to "the algorithm" this morning. I though I also added a comment that I use this algorithm (generate a string of fixed length from a set of characters) since around 2005 to generate initial passwords in PHP apps - but not seeing it anymore, maybe I fogot to hit click a button while I got distracted with other stuff.

[SaschaSchwarze0]

@gabemontero here you go.

/unhold

[gabemontero]

/lgtm