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

Adding VolumeClaimTemplate to pipelines-tutorial #95

Merged

Conversation

VeereshAradhya
Copy link
Contributor

  • Added little info about VolumeClaimTemplate and updated the tkn command in README.md to use volumeClaimTemplate
  • Updated demo.sh to use VolumeClaimTemplate for the vote-ui pipelinerun

Copy link
Contributor

@savitaashture savitaashture left a comment

Choose a reason for hiding this comment

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

Thank you 👍


```bash
$ tkn pipeline start build-and-deploy \
-w name=shared-workspace,claimName=source-pvc \
-w name=shared-workspace,volumeClaimTemplateFile=https://raw.githubusercontent.com/openshift/pipelines-tutorial/master/01_pipeline/03_persistent_volume_claim.yaml \
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do the same changes here https://github.com/openshift/pipelines-tutorial/blob/master/02_pipelinerun/02_build_deploy_ui_pipelinerun.yaml#L17-L18 so that it will consistent while using tkn or yaml for vote-ui

@piyush-garg
Copy link
Contributor

I think we can use volumeClaimTemplate in both pipelinerun, this means we can remove PVC yaml also update the docs accordingly

@VeereshAradhya
Copy link
Contributor Author

@piyush-garg The idea is to use both VolumeClaimTemplate and PersistentVolumeClaim in the tutorial and by doing that we are providing example usages of both VolumeClaimTemplate and PersistentVolumeClaim to the user

@khrm
Copy link
Contributor

khrm commented Sep 8, 2020

I think we try to keep tutorial simple. That's why we will move to single git repo example. So having only one approach is fine too.

@VeereshAradhya
Copy link
Contributor Author

@Preeticp @savitaashture WDYT? If you all agree I will update the PR to contain only VolumeClaimTemplate

@savitaashture
Copy link
Contributor

savitaashture commented Sep 9, 2020

Actually it will be good if we show how we can use VolumeClaimTemplate and PersistentVolumeClaim
but if we are considering to keep tutorial simple then it make sense to use VolumeClaimTemplate 👍

+1 to use VolumeClaimTemplate

Copy link
Contributor

@savitaashture savitaashture left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Minor change

@@ -45,7 +45,7 @@ The custom resources needed to define a pipeline are listed below:
In short, in order to create a pipeline, one does the following:
* Create custom or install [existing](https://github.com/tektoncd/catalog) reusable `Tasks`
* Create a `Pipeline` and `PipelineResources` to define your application's delivery pipeline
* Create a `PersistentVolumeClaim` to provide the volume/filesystem for pipeline execution
* Create a `PersistentVolumeClaim` to provide the volume/filesystem for pipeline execution or provide a `VolumeClaimTemplate` which creates a `PersistentVolumeClaim`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to change this sentence bit because we are not creating PersistentVolumeClaim now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just extra information. Anyway even VolumeClaimTemplate creates a PVC. I think it's ok to keep it :)

@piyush-garg
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link

@piyush-garg: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

@savitaashture savitaashture 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-robot
Copy link

@savitaashture: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@vdemeester vdemeester 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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: piyush-garg, savitaashture, vdemeester, VeereshAradhya

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 Sep 16, 2020
@openshift-merge-robot openshift-merge-robot merged commit b2a4d5d into openshift:master Sep 16, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants