Skip to content

RHDEVDOCS 5244 Tekton Results documentation #63780

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
Closed
wants to merge 1 commit into from
Closed

RHDEVDOCS 5244 Tekton Results documentation #63780

wants to merge 1 commit into from

Conversation

mramendi
Copy link
Contributor

@mramendi mramendi commented Aug 22, 2023 •
edited
Loading

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Aug 22, 2023 •
edited
Loading

🤖 Updated build preview is available at:
https://63780--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/25825

@openshift-ci openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2023
avinal
avinal suggested changes Sep 8, 2023
View reviewed changes
Copy link
Member

@avinal avinal left a comment

Choose a reason for hiding this comment

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

Few things that, I think, were misunderstood or not clear:

  • Both Results and Records can be of type PipelineRun or TaskRun, let's say there is a PR with 1 TR then the Result entry will have 2 Records associated with it. if there is a TR only, the Result will have only 1 record. Similary if logging is enabled, more records (of type log) will be associated.
  • Result/Record UUID is not related to PR/TR UUID. But Name of the Result/Record is created using PR/TR UUID.

navyatatineni
navyatatineni reviewed Sep 8, 2023
View reviewed changes
navyatatineni
navyatatineni reviewed Sep 8, 2023
View reviewed changes
avinal
avinal reviewed Sep 11, 2023
View reviewed changes
Copy link
Member

@avinal avinal 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 from my side. Few more suggestions.

ppitonak
ppitonak reviewed Sep 11, 2023
View reviewed changes
avinal
avinal reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@avinal avinal left a comment

Choose a reason for hiding this comment

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

Rest looks fine to me

avinal
avinal suggested changes Sep 15, 2023
View reviewed changes
Copy link
Member

@avinal avinal left a comment

Choose a reason for hiding this comment

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

Few more changes.

ppitonak
ppitonak reviewed Sep 20, 2023
View reviewed changes
avinal
avinal reviewed Sep 21, 2023
View reviewed changes
khrm
khrm reviewed Sep 21, 2023
View reviewed changes
ppitonak
ppitonak reviewed Sep 22, 2023
View reviewed changes
sferich888
sferich888 reviewed Sep 25, 2023
View reviewed changes
sferich888
sferich888 suggested changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@sferich888 sferich888 left a comment

Choose a reason for hiding this comment

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

We should't link to yq (as its not Red Hat distributed code).
Note: everything you do with yq could be done with other bash (or RHEL provided programs).

khrm
khrm reviewed Sep 25, 2023
View reviewed changes
khrm
khrm reviewed Sep 25, 2023
View reviewed changes
ppitonak
ppitonak suggested changes Sep 25, 2023
View reviewed changes
ppitonak
ppitonak approved these changes Sep 25, 2023
View reviewed changes
avinal
avinal approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@avinal avinal 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

@mramendi
Copy link
Contributor Author

/label peer-review-needed

@mramendi mramendi changed the title WIP: RHDEVDOCS 5244 Tekton Results documentation RHDEVDOCS 5244 Tekton Results documentation Sep 25, 2023
@openshift-ci openshift-ci bot added peer-review-needed Signifies that the peer review team needs to review this PR and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 25, 2023
@adellape adellape added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Sep 25, 2023
@adellape adellape self-assigned this Sep 25, 2023
adellape
adellape reviewed Sep 25, 2023
View reviewed changes
Copy link
Contributor

@adellape adellape left a comment

Choose a reason for hiding this comment

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

Reminder to squash to 1 commit before merge-review, as well. Thank you!

khrm
khrm reviewed Sep 26, 2023
View reviewed changes
Copy link

@khrm khrm 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-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2023
@openshift-merge-robot
Copy link

PR needs rebase.

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.

ppitonak
ppitonak reviewed Sep 26, 2023
View reviewed changes
@adellape adellape added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 26, 2023
@adellape adellape added this to the Continuous Release milestone Sep 26, 2023
@mramendi mramendi mentioned this pull request Sep 26, 2023
Merged
1 task
khrm
khrm reviewed Sep 26, 2023
View reviewed changes
modules/op-installing-results.adoc
----
db_host: postgres.internal.example.com # <1>
db_port: 5432 # <2>
db_user: user # <3>
Copy link

Choose a reason for hiding this comment

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

This isn't required. Please remove this. It's part of secret.

khrm
khrm reviewed Sep 26, 2023
View reviewed changes
modules/op-installing-results.adoc
logs_api: true
log_level: debug
db_port: 5432
db_user: result
Copy link

Choose a reason for hiding this comment

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

Please remove from here also.

@mramendi mramendi closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

7 more reviewers

@ppitonak ppitonak ppitonak approved these changes

@khrm khrm khrm left review comments

@adellape adellape adellape left review comments

@navyatatineni navyatatineni navyatatineni left review comments

@sferich888 sferich888 sferich888 requested changes

@vdemeester vdemeester vdemeester approved these changes

@avinal avinal avinal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adellape adellape

Labels

branch/enterprise-4.12 branch/enterprise-4.13 branch/enterprise-4.14 needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. peer-review-done Signifies that the peer review team has reviewed this PR size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

Continuous Release

Development

Successfully merging this pull request may close these issues.

10 participants

@mramendi @ocpdocs-previewbot @openshift-merge-robot @vdemeester @ppitonak @khrm @sferich888 @adellape @navyatatineni @avinal