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.

Sign up for GitHub

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

Jump to bottom

Cut PipelineResources from some examples #4971

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Cut PipelineResources from some examples #4971

merged 1 commit into from
Jun 22, 2022

Conversation

lbernick
Copy link
Member

@lbernick lbernick commented Jun 13, 2022 •
edited
Loading

This commit replaces PipelineResources with workspaces in documentation examples
not related to those features. PipelineResources are deprecated, so our documentation shouldn't be
using them as examples unless it is necessary.

It also cuts the example related to reusing a Task with different
PipelineResources (as the recommended way to do this would be with params).

Lastly, it encourages users to reference the community catalog, rather
than examples tests, for examples of Tasks, because many examples tests
either make use of PipelineResources or aren't realistic.

/kind documentation

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • n/a Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in
    (if there are no user facing changes, use release note "NONE")

Release Notes

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/documentation Categorizes issue or PR as related to documentation. labels Jun 13, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 13, 2022
@abayer
Copy link
Contributor

abayer commented Jun 14, 2022

/lgtm
/retest

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2022
@lbernick
Copy link
Member Author

/test pull-tekton-pipeline-alpha-integration-tests

@lbernick
Copy link
Member Author

closes #4375

pritidesai
pritidesai reviewed Jun 14, 2022
View reviewed changes
docs/tasks.md Outdated
@@ -34,8 +34,6 @@ weight: 200
- [Substituting in `Script` blocks](#substituting-in-script-blocks)
- [Code examples](#code-examples)
- [Building and pushing a Docker image](#building-and-pushing-a-docker-image)
- [Mounting multiple `Volumes`](#mounting-multiple-volumes)
Copy link
Member

Choose a reason for hiding this comment

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

Can we wait until we have approval on tektoncd/community#720?

@pritidesai
Copy link
Member

It also cuts the example related to reusing a Task with different
PipelineResources (as the recommended way to do this would be with
params), and the Task examples of mounting multiple Volumes or mounting
a ConfigMap as a volumeSource, as these examples are covered by
workspaces documentation.

What is the best practice to configure secrets in a task which is provides the same level of isolation as volumes?

@abayer
Copy link
Contributor

abayer commented Jun 15, 2022

/retest

@afrittoli
Copy link
Member

/test pull-tekton-pipeline-alpha-integration-tests

afrittoli
afrittoli reviewed Jun 17, 2022
View reviewed changes
docs/pipelines.md
@@ -83,7 +83,7 @@ A `Pipeline` definition supports the following fields:
- [`tasks`](#adding-tasks-to-the-pipeline) - Specifies the `Tasks` that comprise the `Pipeline`
and the details of their execution.
- Optional:
- [`resources`](#specifying-resources) - **alpha only** Specifies
- [`resources`](#specifying-resources) - **deprecated** Specifies
Copy link
Member

Choose a reason for hiding this comment

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

🎉

afrittoli
afrittoli reviewed Jun 17, 2022
View reviewed changes
docs/tasks.md Outdated
type: Socket
```

#### Mounting multiple `Volumes`
Copy link
Member

Choose a reason for hiding this comment

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

As long as the feature is still there perhaps we shouldn't fully remove this docs, but alter them to say that while this is possible, it is not recommended unless in use cases where no alternative exists.

@afrittoli
Copy link
Member

Thanks for this @lbernick.
Perhaps we could split the PipelineResources part and the volume one, so we could merge the first part right away.
If you prefer to wait for the volume part to be ready that is also ok :)

@lbernick
Copy link
Member Author

Perhaps we could split the PipelineResources part and the volume one, so we could merge the first part right away. If you prefer to wait for the volume part to be ready that is also ok :)

Thanks @afrittoli and @pritidesai, I think I'll make this PR just PipelineResources for now

@lbernick
Cut PipelineResources from some examples
e5a8584 
This commit replaces PipelineResources with
workspaces in documentation examples not related to those features.
PipelineResources are deprecated, so our documentation shouldn't be
using them as examples unless it is necessary.

It also cuts the example related to reusing a Task with different
PipelineResources (as the recommended way to do this would be with
params).

Lastly, it encourages users to reference the community catalog, rather
than examples tests, for examples of Tasks, because many examples tests
either make use of PipelineResources or aren't realistic.
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022
@lbernick lbernick changed the title Cut PipelineResources + Volumes from some examples Cut PipelineResources from some examples Jun 17, 2022
@lbernick
Copy link
Member Author

Ok, should be ready for re-review

dibyom
dibyom reviewed Jun 17, 2022
View reviewed changes
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2022
@jerop jerop closed this Jun 22, 2022
@jerop jerop reopened this Jun 22, 2022
@tekton-robot tekton-robot merged commit bb228db into tektoncd:main Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dibyom dibyom dibyom left review comments

@afrittoli afrittoli afrittoli left review comments

@pritidesai pritidesai pritidesai left review comments

@vdemeester vdemeester vdemeester approved these changes

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

Assignees

@abayer abayer

@dibyom dibyom

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lbernick @abayer @pritidesai @afrittoli @tekton-robot @vdemeester @dibyom @jerop