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

Automate Pull Request Checklist - Check Tasks Completed #911

Merged
merged 1 commit into from
Jan 10, 2022

Conversation

jerop
Copy link
Member

@jerop jerop commented Oct 21, 2021

Changes

As described in #910, we have a submitter checklists in pull requests that
enforce with our standards.

Currently, maintainers and reviewers have to manually check that the
submitter checklist has been checked off and verify each item in the
list. If the submitter checklist has not been checked off, they need
to follow up with the user and request them to follow the standards
and check them off.

In this change, we add automation that checks that the checklist has
been checked off. If there's an incomplete item, the check reports that
there are incomplete tasks but does not fail yet. In a future change, we
will make the check fail if the submitter checklist is not checked off.
We hope that enforcing checking off the submitter checklist will encourage
contributors to look into our standards and apply them to their
contributions.

The implementation is modeled after the check-kind-label job.

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.

@jerop jerop linked an issue Oct 21, 2021 that may be closed by this pull request
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2021
@jerop jerop added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 21, 2021
@dibyom dibyom self-assigned this Oct 25, 2021
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.

Overall this looks great! I have a couple of general comments (mostly about the layout of the resources) which do not have to be addressed in this PR:

  1. I think we will be creating two pipelineruns with conditions for each event - and then in each condition we filter out to see if the command name from the PR matches the name of the check. I wonder if we can do this filtering either using a CEL expression in Triggers or in pipelines using a when expression so we can reduce the overhead of the extra pod being created.

  2. We are adding a single new binding -- looks like we are using it in a bunch of places. Wonder if there is a way to reduce this boilerplate

  3. This will suffer from the same race condition as the the existing check-labels run though I think we should tackle those outside this PR as part of Improve reliability of the check-pr-has-kind-label job  #888 or How to run TaskRuns in non-parallel mode pipeline#3989 (comment)

Would be nice to get @afrittoli 's comments on these as well!

/lgtm

tekton/ci/jobs/tasks.yaml Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2021
Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

I wonder if we can do this filtering either using a CEL expression in Triggers

I think this same approach is being used for all the triggering - and it seems like the motivation is to be able to have one TriggerTemplate per repo being triggered (e.g. https://github.com/tektoncd/plumbing/blob/main/tekton/ci/templates/plumbing-template.yaml is the template for the plumbing repo) - the CEL expressions filter by repo but then leave it up to the pipelines to decide if they should continue running.

or in pipelines using a when expression so we can reduce the overhead of the extra pod being created.

At the moment the entire comment left on github is being passed to the pipeline so it needs to extract the "command" part of that which atm would still require a pod (or a CELRun :D). Maybe the trigger configuration could do the extraction and pass only the "check name" down to the pipelines - then all we'd need is a when expression!

tekton/ci/jobs/tekton-github-tasks-completed.yaml Outdated Show resolved Hide resolved
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!
I agree with @bobcatfish - I would prefer this to be in the ci-workspace folder.
Apart from that it looks good. As long as it does not fail and no comment is published, it won't really have an impact on users, but it's fine to add this incrementally.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2021
@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 16, 2021
@jerop jerop marked this pull request as draft December 16, 2021 15:20
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 16, 2021
@jerop jerop force-pushed the github-tasks-completed branch 2 times, most recently from db54892 to 3900da1 Compare December 16, 2021 16:25
@tekton-robot tekton-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 16, 2021
@jerop jerop marked this pull request as ready for review December 20, 2021 19:01
@jerop
Copy link
Member Author

jerop commented Dec 20, 2021

@afrittoli @bobcatfish @dibyom @vdemeester thank you for the initial reviews on this PR - it is ready for another look

As described in tektoncd#910, we
have a submitter checklists in pull requests that enforce with our
standards.

Currently, maintainers and reviewers have to manually check that the
submitter checklist has been checked off and verify each item in the
list. If the submitter checklist has not been checked off, they need
to follow up with the user and request them to follow the standards
and check them off.

In this change, we add automation that checks that the checklist has
been checked off. If there's an incomplete item, the check reports that
there are incomplete tasks but does not fail yet. In a future change, we
will make the check fail if the submitter checklist is not checked off.
We hope that enforcing checking off the submitter checklist will encourage
contributors to look into our standards and apply them to their
contributions.

The implementation is modeled after the check-kind-label job.
@ghost
Copy link

ghost commented Dec 21, 2021

Thanks for creating the env var issue!

/lgtm

@tekton-robot
Copy link
Contributor

@sbwsg: changing LGTM is restricted to collaborators

In response to this:

Thanks for creating the env var issue!

/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.

@ghost
Copy link

ghost commented Dec 23, 2021

Let's try this again:

/lgtm

@tekton-robot tekton-robot assigned ghost Dec 23, 2021
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2021
@lbernick
Copy link
Member

lgtm!

@bobcatfish
Copy link
Contributor

Thanks for putting this together @jerop ! Very excited to see it in action :D

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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 Jan 10, 2022
@tekton-robot tekton-robot merged commit a749e56 into tektoncd:main Jan 10, 2022
jerop added a commit to jerop/plumbing that referenced this pull request Jan 13, 2022
We just added a Tekton Task that checks that the
GitHub Tasks have been completed in pull requests.

In this change, we:
* fix the namespace - it's `tekton-ci` not `tektonci`
* import os in the python script

Related PR:
* tektoncd#911
@jerop jerop mentioned this pull request Jan 13, 2022
2 tasks
jerop added a commit to jerop/plumbing that referenced this pull request Jan 13, 2022
We just added a Tekton Task that checks that the
GitHub Tasks have been completed in pull requests.

In this change, we:
* fix the namespace - it's `tekton-ci` not `tektonci`
* import os in the python script
* fix the query string, needs space between brackets

Related PR:
* tektoncd#911
jerop added a commit to jerop/plumbing that referenced this pull request Jan 13, 2022
We just added a Tekton Task that checks that the
GitHub Tasks have been completed in pull requests.

In this change, we:
* fix the namespace - it's `tekton-ci` not `tektonci`
* import os in the python script
* fix the query string, needs space between brackets

Related PR:
* tektoncd#911
tekton-robot pushed a commit that referenced this pull request Jan 14, 2022
We just added a Tekton Task that checks that the
GitHub Tasks have been completed in pull requests.

In this change, we:
* fix the namespace - it's `tekton-ci` not `tektonci`
* import os in the python script
* fix the query string, needs space between brackets

Related PR:
* #911
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate Pull Request Checklist
8 participants