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 EP on Build Strategies Parametrization #697

Merged

Conversation

qu1queee
Copy link
Contributor

Changes

Provides an EP on how to manage parameters in Strategies, by providing more flexibility to Shipwright users and strategy administrators.

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 EP on Parameterize Build Strategies

Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Nice. Bringing some suggestions and aspects for discussion.

docs/proposals/parameterize-strategies.md Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Outdated Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Outdated Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Outdated Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Show resolved Hide resolved
docs/proposals/parameterize-strategies.md Show resolved Hide resolved
@adambkaplan
Copy link
Member

@qu1queee do we need to reach a decision in #694 to make progress on this EP, or is this a related issue that you uncovered drafting the issue?

@gabemontero
Copy link
Member

gabemontero commented Apr 7, 2021

Quick FYI, I've been asked to take a look at #224 and compose an EP for that.

Now, while #224 gets into treating env vars as a "first class citizen" in the api, vs. just a subset of parameters, my thought is that this EP can still be a building block or prereq for an EP written for env vars in that we would want to look at supporting parameterization/perform variable substitution/provide defaults for env vars like what is available for Tekton.

In my initial planning for the EP, I'm thinking I'm going to call this one out as a "dependency" in some fashion, even though separate APIs will arise to satisfy the "first class treatment" @sbose78 called for.

Anyway, I'm not saying this EP should reference "env var scenarios" specifically. Just wanted to mention here what has been alluded to in other issues, etc. as well as I think the community call, in that there is "a connection". Certainly the motivations @qu1queee mentioned up top in this EP lend themselves minimally to what Tekton does with env vars today, at least IMO.

And of course opinions on this dependency notion, connection, etc. are welcome.

That said, if you want @qu1queee to call out the "env var scenario" in some fashion here I certainly won't object.

Some quick references I'm sure the folks already commenting here are aware of:

Just refreshed my memory with the tekton code: the tekton pipline taskrun reconciler performs variable substitution on the taskrun.taskspec.step/container.envvar fields; As shipwright generates a task run with a task spec, would can leverage tekton's support here. Will include this in my upcoming ep.

Just refreshed my memory with the tekton code: step templates are part of the task spec which is what shipwright manipulates when building a task run. So, the defaulting notion provided by step templates also seems to be something we can exploit. Shipwright's task spec generation is not generating a spec template yet, but I see no reason in the current code why it could not.

(you see a direct use of params to seed the k8s valueFrom there)

@qu1queee qu1queee force-pushed the qu1queee/ep_params branch 4 times, most recently from c456624 to 670e628 Compare April 20, 2021 10:02
@qu1queee
Copy link
Contributor Author

@adambkaplan this EP assumes the decision is to expose the Tekton nomenclature.

@qu1queee
Copy link
Contributor Author

@gabemontero thanks, nice summary. I think we can keep this EP without a notion of env vars, but I definitely see this the other way around, where env vars EP should reference this one, mainly because of the variable substitution, where a var comes for a parameter definition.

Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot
Copy link

[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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2021
This covers the use case introduced in issue
shipwright-io#184

This also will provide an enhancement in the current Build API, by
migrating existing fields into a more readable/understandable path.
@imjasonh
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2021
@openshift-merge-robot openshift-merge-robot merged commit aa682c7 into shipwright-io:master Apr 20, 2021
@gabemontero
Copy link
Member

@gabemontero thanks, nice summary. I think we can keep this EP without a notion of env vars, but I definitely see this the other way around, where env vars EP should reference this one, mainly because of the variable substitution, where a var comes for a parameter definition.

sounds good @qu1queee

and my EP draft #726 in fact does reference this one

thanks

@qu1queee qu1queee deleted the qu1queee/ep_params branch May 31, 2021 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants