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

Catlin: Report logs back to the PR after execution #758

Merged

Conversation

vinamra28
Copy link
Member

Changes

As of now catlin is running on PRs of catalog and validate them as per
TEP-0003 but there are some warnings which are produced by catlin which
maybe left un-noticed by the PR author as catlin check doesn't fails.

This PR adds a script which will search whether catlin has produced any
warning or error and if found will post the output as comment on the PR.

/kind feature

Signed-off-by: vinamra28 vinjain@redhat.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 4, 2021
@tekton-robot tekton-robot requested review from a user and wlynch March 4, 2021 18:12
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 4, 2021
Copy link
Member Author

@vinamra28 vinamra28 left a comment

Choose a reason for hiding this comment

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

also please do tell where to document that github-add-comment task is being used from tektoncd/catalog 😅

Comment on lines +110 to +123
- name: GITHUB_TOKEN_SECRET_NAME
value: bot-token-github
- name: GITHUB_TOKEN_SECRET_KEY
value: bot-token
Copy link
Member Author

Choose a reason for hiding this comment

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

is there a secret setup which contains the base64 personal access token? If yes, then please help me with the correct values that I can substitute over here 🙂

Copy link
Member

Choose a reason for hiding this comment

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

The values you have in there are already ok

@vinamra28
Copy link
Member Author

/cc @afrittoli @vdemeester @piyush-garg

@wlynch wlynch removed their request for review March 4, 2021 18:18
@vinamra28 vinamra28 force-pushed the vinamra28/add-catlin-result-as-comment branch 2 times, most recently from 69a03bd to 4fa3e57 Compare March 4, 2021 18:27
@vinamra28
Copy link
Member Author

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2021
@vinamra28 vinamra28 force-pushed the vinamra28/add-catlin-result-as-comment branch 2 times, most recently from acbc533 to 2cd0cf3 Compare March 7, 2021 17:17
@vinamra28
Copy link
Member Author

/hold cancel
but this requires tektoncd/catalog#664 to be merged first 😅

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2021
@vinamra28
Copy link
Member Author

@afrittoli since tektoncd/catalog#664 is now merged, can you please tell where to document that I need this task from upstream catalog and how to move forward with this PR?

@vdemeester
Copy link
Member

/test .*

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!
It looks mostly ok, but I think there may be a problem in the script logic.

Comment on lines +110 to +123
- name: GITHUB_TOKEN_SECRET_NAME
value: bot-token-github
- name: GITHUB_TOKEN_SECRET_KEY
value: bot-token
Copy link
Member

Choose a reason for hiding this comment

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

The values you have in there are already ok

Comment on lines 57 to 66
# if there is any ERROR or WARN then post the comment else skip
[[ $isWarning -eq 0 ]] && [[ $isError -eq 1 ]] && \
echo -n "" > $(workspaces.store-changed-files.path)/catlin.txt
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand the logic here, perhaps it should be $isError -eq 0?

BTW, it looks like the post-comment will be executed regardless, only with an empty comment?
Hopefully the github API is smart enough to not post an empty comment?

A nice solution would be to have a result and use when to skip the comment if not needed. Unfortunately when expressions are not available in finally yet (tektoncd/community#240).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really understand the logic here, perhaps it should be $isError -eq 0?

oh yes 😅, it should be $isError -eq 0. Will fix this 😅

BTW, it looks like the post-comment will be executed regardless, only with an empty comment?
Hopefully the github API is smart enough to not post an empty comment?

yeah when there is an empty comment then no comment would be posted on PR

A nice solution would be to have a result and use when to skip the comment if not needed. Unfortunately when expressions are not available in finally yet (tektoncd/community#240).

Copy link
Member Author

Choose a reason for hiding this comment

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

btw the following script will produce the output as


Catlin Output

FILE: task/black/0.1/black.yaml
WARN : Step "format-python-code" uses image "$(params.image)" that contains variables; skipping validation
FILE: task/yaml-lint/0.1/yaml-lint.yaml

@vinamra28 vinamra28 force-pushed the vinamra28/add-catlin-result-as-comment branch from 2cd0cf3 to 92a0609 Compare March 18, 2021 14:23
tekton/ci/jobs/tekton-catlin-lint.yaml Outdated Show resolved Hide resolved
tekton/ci/jobs/tekton-catlin-lint.yaml Outdated Show resolved Hide resolved
As of now catlin is running on PRs of catalog and validate them as per
TEP-0003 but there are some warnings which are produced by catlin which
maybe left un-noticed by the PR author as catlin check doesn't fails.

This PR adds a script which will search whether catlin has produced any
warning or error and if found will post the output as comment on the PR.

Signed-off-by: vinamra28 <vinjain@redhat.com>
@vinamra28 vinamra28 force-pushed the vinamra28/add-catlin-result-as-comment branch from 92a0609 to 24e5a21 Compare March 23, 2021 15:15
@vinamra28
Copy link
Member Author

/cc @afrittoli

@ghost
Copy link

ghost commented Mar 23, 2021

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, 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

@ghost
Copy link

ghost commented Mar 24, 2021

/lgtm

@tekton-robot tekton-robot assigned ghost Mar 24, 2021
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2021
@tekton-robot tekton-robot merged commit e736c7b into tektoncd:main Mar 24, 2021
@vinamra28 vinamra28 deleted the vinamra28/add-catlin-result-as-comment branch March 24, 2021 12:46
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. 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.

5 participants