Skip to content

Docs: Add TaskRun Status documentation section#7147

Merged
tekton-robot merged 1 commit intotektoncd:mainfrom
HamzaMateen:docs/taskruns-status-section
Oct 3, 2023
Merged

Docs: Add TaskRun Status documentation section#7147
tekton-robot merged 1 commit intotektoncd:mainfrom
HamzaMateen:docs/taskruns-status-section

Conversation

@HamzaMateen
Copy link
Contributor

@HamzaMateen HamzaMateen commented Sep 25, 2023

[Issue Link: #5720]

Description:

This PR addresses the issue by adding a new section to the docs/taskruns.md file to document the fields in TaskRun status. The structure of the new section closely follows that of the PipelineRun status documentation, with Required and Optional sections.

Changes Made:

  • Added a new section to docs/taskruns.md for TaskRun Status documentation.
  • Documented the key fields in TaskRun status, including their descriptions and significance.
  • Ensured consistency and clarity in the documentation, aligning it with the existing PipelineRun documentation style.

Files Changed

  • docs/taskruns.md

Testing Done:

Verified the changes by building the documentation locally and reviewing the updated taskruns.md file. Ensured that the new section follows the established documentation structure.

Screenshots:

N/A

Checklist:

  • I have followed the guidelines in contributing.md for documentation contributions.
  • I have linked this PR to the related issue (#<issue_number>).
  • The documentation changes are technically accurate and easy to understand.
  • I have tested the changes locally and ensured they display correctly in the documentation.
  • I have reviewed and proofread the documentation to eliminate errors or inconsistencies.
  • All commits are signed-off (verified using git commit -s).
  • The PR is ready for review and merge.

Fixes #5720

Release Notes

NONE

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 25, 2023
@tekton-robot
Copy link
Collaborator

Hi @HamzaMateen. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@HamzaMateen
Copy link
Contributor Author

/kind documentation

@tekton-robot tekton-robot added kind/documentation Categorizes issue or PR as related to documentation. release-note-none Denotes a PR that doesnt merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 25, 2023
@afrittoli
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 25, 2023
docs/taskruns.md Outdated
- `RefSource`: the source from where a remote `Task` definition was fetched.
- `FeatureFlags`: Identifies the feature flags used during the `TaskRun`.
- `Step` - Reports the results of running a `step` in a `Task`.
- `Retries` - Contains the history of `TaskRun`'s `status` in case of a retry in order to keep record of failures.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is retriesStatus

docs/taskruns.md Outdated
- Optional:
- `taskResults`- Describes the results of this `task`.

- `Provenance` - Provenance contains metadata about resources used in the `TaskRun` such as the source from where a remote `task` definition was fetched. It carries minimum amount of metadata in `TaskRun` `status` so that `Tekton Chains` can utilize it for provenance, its two subfields are:
Copy link
Member

Choose a reason for hiding this comment

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

provenance (lower case)

Copy link
Member

Choose a reason for hiding this comment

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

The go types are upper case, but the yaml/json rendering is lower case. Same goes for refSource, featureFlags, sidecars, etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this comment made everything clear.
I couldn't think through this naming used, now I have some idea. thanks

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thanks for this!
It looks mostly good - I left a couple of comments.

docs/taskruns.md Outdated
- Optional:
- `taskResults`- Describes the results of this `task`.

- `Provenance` - Provenance contains metadata about resources used in the `TaskRun` such as the source from where a remote `task` definition was fetched. It carries minimum amount of metadata in `TaskRun` `status` so that `Tekton Chains` can utilize it for provenance, its two subfields are:
Copy link
Member

Choose a reason for hiding this comment

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

The go types are upper case, but the yaml/json rendering is lower case. Same goes for refSource, featureFlags, sidecars, etc...

The `TaskRun`'s `status` field can contain the following fields:

- Required:
- `status` - The most relevant information about the TaskRun's state. This field includes:
Copy link
Member

Choose a reason for hiding this comment

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

NIT: I think the "most relevant" comment is not necessary, but it's also ok to keep

### The `status` field
The `TaskRun`'s `status` field can contain the following fields:

- Required:
Copy link
Member

Choose a reason for hiding this comment

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

podName is missing in the "Required" section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I generated the API spec using Swagger, I found out that podName was required but default so I skipped it. I will add it in my next edition. Thanks

docs/taskruns.md Outdated
- [`taskSpec`](tasks.md#configuring-a-task) - `TaskSpec` defines the desired state of the `Task` executed via the `TaskRun`.

- Optional:
- `taskResults`- Describes the results of this `task`.
Copy link
Member

Choose a reason for hiding this comment

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

This should be results.
NIT: I would say "contains" instead of "describes" - there is no description really. it's their names and values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

docs/taskruns.md Outdated
- `FeatureFlags`: Identifies the feature flags used during the `TaskRun`.
- `Step` - Reports the results of running a `step` in a `Task`.
- `Retries` - Contains the history of `TaskRun`'s `status` in case of a retry in order to keep record of failures.
- [`Sidecar`](tasks.md#using-a-sidecar-in-a-task) - Reports the results of running a `sidecar` in a `Task`.
Copy link
Member

Choose a reason for hiding this comment

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

sidecars (plural). This is the comment in the code:

The list has one entry per sidecar in the manifest. Each entry represents the imageid of the corresponding sidecar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you please elaborate a bit more on this? should I write this line instead of the one already there?

Copy link
Member

Choose a reason for hiding this comment

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

I just wanted to point out that it can be multiple sidecars, so maybe you could reuse part of that wording

- `completionTime` - The time at which the `TaskRun` finished executing, conforms to [RFC3339](https://tools.ietf.org/html/rfc3339) format.
- [`taskSpec`](tasks.md#configuring-a-task) - `TaskSpec` defines the desired state of the `Task` executed via the `TaskRun`.

- Optional:
Copy link
Member

Choose a reason for hiding this comment

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

spanContext is missing in this list

docs/taskruns.md Outdated
specifying a `ServiceAccount` object name in the `serviceAccountName` field in your `TaskRun`
definition. If you do not explicitly specify this, the `TaskRun` executes with the credentials
specified in the `configmap-defaults` `ConfigMap`. If this default is not specified, `TaskRuns`
specified in the `configmap-defaults` `ConfigMap`. If this defaulPipelineRunt is not specified, `TaskRuns`
Copy link
Member

Choose a reason for hiding this comment

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

I guess this was changed by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed!

@HamzaMateen
Copy link
Contributor Author

I will edit this PR and send my changes as soon as I am done with them. Thanks for your thorough review, appreciated!

HamzaMateen added a commit to HamzaMateen/pipeline that referenced this pull request Sep 29, 2023
In response to feedback received on PR tektoncd#7147, this commit addresses the following changes:
- Updated the TaskRun status documentation based on reviewer comments.
- Clarified descriptions for certain fields to improve understanding.
- Ensured consistency in formatting and terminology throughout the TaskRun status section.
- Fixed some typos in corresponding PipelineRuns section for consistency.

Files Changed:
taskruns.md
pipelineruns.md

These changes enhance the clarity and completeness of the TaskRun status documentation, providing users with accurate and detailed information.

Fixes tektoncd#5720
@afrittoli afrittoli changed the title WIP docs: Add TaskRun Status documentation section Docs: Add TaskRun Status documentation section Oct 2, 2023
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2023
Copy link
Member

@afrittoli afrittoli 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 for the updates, it lgtm!

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Oct 2, 2023
docs/taskruns.md Outdated
- [`taskSpec`](tasks.md#configuring-a-task) - `TaskSpec` defines the desired state of the `Task` executed via the `TaskRun`.

- Optional:
- `taskResults`- Contains the results of this `task`.
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be just results?

Results []TaskRunResult `json:"results,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I will update it again. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! lmk when it's updated and we can merge this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update done! (the commit message isn't very accurate) but I didn't see the point of changing it for just a typo.

Copy link
Member

@Yongxuanzhang Yongxuanzhang Oct 3, 2023

Choose a reason for hiding this comment

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

Update done! (the commit message isn't very accurate) but I didn't see the point of changing it for just a typo.

Thanks! Would you mind squashing the 3 commits? This should be the one last thing to merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

HamzaMateen added a commit to HamzaMateen/pipeline that referenced this pull request Oct 3, 2023
In response to feedback received on PR tektoncd#7147, this commit addresses the following changes:
- Updated the TaskRun status documentation based on reviewer comments.
- Clarified descriptions for certain fields to improve understanding.
- Ensured consistency in formatting and terminology throughout the TaskRun status section.
- Fixed some typos in corresponding PipelineRuns section for consistency.

Files Changed:
taskruns.md
pipelineruns.md

Fixes: tektoncd#5720
In response to issue tektoncd#5720, this commit adds a new section to the docs/taskruns.md file to document the fields in TaskRun status in detail. The new section follows a similar structure to the PipelineRun status documentation and is divided into Required and Optional sections.

- The Required section provides a comprehensive list of essential fields in TaskRun status, offering users insights into the key aspects of a TaskRun's state.
- The Optional section complements the Required section by listing additional fields that provide more context and flexibility for TaskRuns.

This addition enhances the documentation and helps users understand the details of TaskRun status, aligning it with the structure and clarity of PipelineRun documentation.

Files changed:
	1. taskruns.md
	2. pipelineruns.md

Fixes: tektoncd#5720
@HamzaMateen HamzaMateen force-pushed the docs/taskruns-status-section branch from 931abae to 13996e7 Compare October 3, 2023 19:40
@Yongxuanzhang
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2023
@tekton-robot tekton-robot merged commit fd4cb46 into tektoncd:main Oct 3, 2023
@afrittoli afrittoli added the hacktoberfest-accepted Categorizes PRs as one accepted Hacktoberfest 2021 label Oct 4, 2023
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. hacktoberfest-accepted Categorizes PRs as one accepted Hacktoberfest 2021 kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesnt merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs/taskruns.md misses a section for fields in status

4 participants