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

SHIP-22: add deprecation notice for implicit tekton workspace emptydir volumes #941

Conversation

gabemontero
Copy link
Member

@gabemontero gabemontero commented Nov 9, 2021

Changes

Announce the deprecation of the implicit use of tekton workspace emptydir volumes for the git repo cited in the build source reference.

Per https://github.com/shipwright-io/community/blob/main/ships/0022-build-strategy-volumes.md#removing-a-deprecated-feature-if-necessary

Per our recent convention, @adambkaplan and I are of the understanding that we announce / "deprecate" in the current release (the upcoming v0.7.0), and then remove / replace with the full implementation of https://github.com/shipwright-io/community/blob/main/ships/0022-build-strategy-volumes.md in v0.8.0

/assign @adambkaplan
@SaschaSchwarze0 @sbose78 @imjasonh FYI (please assign yourselves as you see fit)

@adambkaplan - as you can tell from the PR, there was not much present to latch onto to announce the deprecation for my assignment here. In fact, I added to the API godoc to "reveal" the implicit use of Tekton's emptydir volume to volume mount mapping to lend context to the "deprecation" notice that followed. If there are spots where this implicit use of emptyDir is discussed in documentation that I have missed that you are aware of, please advise. I similarly added a new section in the build strategy markdown to first lend some context, then announce the deprecation.

/kind feature

Submitter Checklist

  • [n/a ] 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

Shipwright's implicit association of an emptydir volume to any BuildStep/Container VolumeSource that needs an associated Volume is marked as deprecated in the upcoming release, with the intent of replacing it with an implementation of SHIP-22 in the following release.  If your build strategies (both Clustered and Namespaced) leverage this behavior, please start planning to employ the alternative approach described in https://github.com/shipwright-io/community/blob/main/ships/0022-build-strategy-volumes.md

@openshift-ci openshift-ci bot added release-note Label for when a PR has specified a release note kind/feature Categorizes issue or PR as related to a new feature. labels Nov 9, 2021
@gabemontero
Copy link
Member Author

Error: error processing import paths in "deploy/500-controller.yaml": error resolving image references: GET https://registry.access.redhat.com/v2/ubi8/ubi-minimal/blobs/sha256:e7685639f55280ffe8da3724cb1d342091a1ceea79af5eb91a07ce97cd32f221: UNAVAILABLE: Server error encountered while handling request
2021/11/09 19:46:29 error during command execution:error processing import paths in "deploy/500-controller.yaml": error resolving image references: GET https://registry.access.redhat.com/v2/ubi8/ubi-minimal/blobs/sha256:e7685639f55280ffe8da3724cb1d342091a1ceea79af5eb91a07ce97cd32f221: UNAVAILABLE: Server error encountered while handling request

assuming that is a registry flake

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.

/reject

SHIP-22 is covering a different deprecation: a build strategy author today can define a volumeMount for any step which results in an emptyDir volume being implicitly added to the TaskRun/Pod. For example: https://github.com/shipwright-io/build/blob/main/samples/buildstrategy/source-to-image/buildstrategy_source-to-image_cr.yaml#L17-L19

The source is handled completely by shipwright. The build strategy author should not be required to define an emptyDir volume for it.

@gabemontero
Copy link
Member Author

gabemontero commented Nov 10, 2021

/reject

SHIP-22 is covering a different deprecation: a build strategy author today can define a volumeMount for any step which results in an emptyDir volume being implicitly added to the TaskRun/Pod. For example: https://github.com/shipwright-io/build/blob/main/samples/buildstrategy/source-to-image/buildstrategy_source-to-image_cr.yaml#L17-L19

The source is handled completely by shipwright. The build strategy author should not be required to define an emptyDir volume for it.

Ahh .... I had mentioned the code links in question where my TODO's currently are with @adambkaplan as part of one of our internal RH meetings where we plan our next assignments, and things were "OK'ed", but looks like something got lost in translation.

Perhaps my elaborations in this PR's description reflected my degree of uncertainty with all this :-)

thanks @SaschaSchwarze0 .... I'll circle back to your reference link and go from there.

@adambkaplan - ^^ ..... I imagine @SaschaSchwarze0 is correct and we had a disconnect in planning and addressing my open questions, but of course chime in if you disagree.

@gabemontero gabemontero force-pushed the deprecate-implicit-emptydir branch from 4ed5a62 to e077c3b Compare November 10, 2021 16:48
@gabemontero gabemontero changed the title SHIP-22: add TODO placeholders and a deprecation notice for implicit tekton workspace emptydir volumes SHIP-22: add deprecation notice for implicit tekton workspace emptydir volumes Nov 10, 2021
@gabemontero
Copy link
Member Author

ok I'm still trying to get confirmation with @adambkaplan on some details, but in the meantime I've taken a stab at resetting how we announce the deprecation @SaschaSchwarze0

if you get the chance let me know if we are on a better path, as well as any specific suggestions etc. on the new text.

Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

In general I think I'm on the same page with @SaschaSchwarze0. It seems that this implicit behavior isn't well documented, so as a first step this PR should:

  1. Document the current behavior
  2. Indicate that this behavior will be replaced in a future release.

@@ -66,6 +66,35 @@ The current supported namespaces BuildStrategy are:
| [buildpacks-v3-heroku](../samples/buildstrategy/buildpacks-v3/buildstrategy_buildpacks-v3-heroku_namespaced_cr.yaml) | linux/amd64 only |
| [buildpacks-v3](../samples/buildstrategy/buildpacks-v3/buildstrategy_buildpacks-v3_namespaced_cr.yaml) | linux/amd64 only |

## Notes on the API for BuildStrategies and ClusterBuildStrategies
Copy link
Member

Choose a reason for hiding this comment

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

I would put this towards the bottom of the article, titled "Volumes and Volume Mounts", and keep things simple. Something to the effect of:

## Volumes and VolumeMounts

Build steps can declare a `volumeMount`, which allows data in the provided path to be shared across build steps.
When a `volumeMount` is declared, Shipwright will create an `emptyDir` volume with the corresponding name.
Build steps whose volume mounts share the same name will share the same underlying `emtpyDir` volume.

In a future release, build strategy authors will be able to use other volume types for the volume mounts.
When this feature is introduced, the volume and volume type will need to be explicitly declared.

pkg/apis/build/v1alpha1/buildstrategy.go Outdated Show resolved Hide resolved
@gabemontero
Copy link
Member Author

In general I think I'm on the same page with @SaschaSchwarze0.

ok good

It seems that this implicit behavior isn't well documented,

yep that is what I alluded to in my updated description and tried to address

so as a first step this PR should:

1. Document the current behavior

yep I've attempted to do that

2. Indicate that this behavior will be replaced in a future release.

yep attempted to do that too

@gabemontero gabemontero force-pushed the deprecate-implicit-emptydir branch from d9651f2 to da6f620 Compare November 10, 2021 22:11
@gabemontero
Copy link
Member Author

suggestions pulled in and squashed @adambkaplan

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. Looks better now @gabemontero. ;-) May you run the generator to update the OpenAPI spec ?

Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/approve

Just needs the CRD to be regenerated.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 12, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2021
@gabemontero
Copy link
Member Author

make generate-crds output pushed as separate commit @SaschaSchwarze0 @adambkaplan thanks

@gabemontero
Copy link
Member Author

/hold

I did the crd update with fedora .... need to redo that with my crd friendly ubuntu

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2021
@gabemontero gabemontero force-pushed the deprecate-implicit-emptydir branch from 8b55b0f to 54eb1e6 Compare November 30, 2021 15:10
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2021
@gabemontero gabemontero force-pushed the deprecate-implicit-emptydir branch from 54eb1e6 to cefb6cb Compare November 30, 2021 15:11
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2021
@gabemontero
Copy link
Member Author

/hold cancel

@SaschaSchwarze0 @adambkaplan the crd line break situation has been addressed

PTAL

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2021
@gabemontero
Copy link
Member Author

/hold cancel

@SaschaSchwarze0 @adambkaplan the crd line break situation has been addressed

PTAL

wait a sec @SaschaSchwarze0 @adambkaplan .... still having verify issues ... will update when I've sorted them out.

@gabemontero gabemontero force-pushed the deprecate-implicit-emptydir branch from 6719266 to 38a4409 Compare November 30, 2021 15:41
@gabemontero
Copy link
Member Author

ok it is all green now @SaschaSchwarze0 @adambkaplan with no preponderance of line breaks .... PTAL

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.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit 06c7d08 into shipwright-io:main Nov 30, 2021
@gabemontero gabemontero deleted the deprecate-implicit-emptydir branch November 30, 2021 17:14
@SaschaSchwarze0 SaschaSchwarze0 added this to the release-v0.7.0 milestone Dec 9, 2021
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. kind/feature Categorizes issue or PR as related to a new feature. 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.

4 participants