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

tide errors: merge requirements do not match github setttings #1653

Closed
BenTheElder opened this issue Jul 23, 2019 · 34 comments · Fixed by kubernetes/test-infra#18095
Closed

Comments

@BenTheElder
Copy link

A maintainer of this repo / the CI setup should configure Prow / Tide (the merge robot) to have accurate merge requirements matching the Required statuses configured on pull requests. AFAIK these need to be kept in sync to work properly (?) cc @cjwagner to confirm...

failed merging [1652]: [PR is unmergable. Do the Tide merge requirements match the GitHub settings for the repo? Required status check "Travis CI - Pull Request" is failing.]

https://prow.k8s.io/tide-history

This is showing on #1652

cc @jlewi for kubeflow prow

@cjwagner
Copy link
Member

That isn't a normal status check, it is a GitHub check (https://github.com/kubeflow/pipelines/pull/1652/checks) which Tide cannot support because it is not a GitHub App.

Can you move the Travis CI tests to a ProwJob?

@gaoning777
Copy link
Contributor

Discussed with cjwagner offline. This issue causes Tide to continually try to merge an unmergable PR which burns through the bots API rate limit.
@jessiezcc

@BenTheElder
Copy link
Author

ping, we still have error spam from this and AFAIK burning bot API rate limit

@cjwagner
Copy link
Member

I also opened an issue about this in march, but I have been struggling to get anyone to address it. #930

@jlewi
Copy link
Contributor

jlewi commented Nov 12, 2019

@gaoning777 @jessiezcc can you please look into this?

@cjwagner @gaoning777 its not clear to me how to how fix this. If you can help me figure out what to do I will be happy to do it.

Here's a script for the list of status checks for the repo.

If I uncheck "Travis CI" will that fix it.
pipelines ?

@Bobgy
Copy link
Contributor

Bobgy commented Nov 12, 2019

I can move frontend part of the travis tests to prow.

@IronPan
Copy link
Member

IronPan commented Nov 12, 2019

@Bobgy Thanks moving travis to prow would be great.

@Bobgy
Copy link
Contributor

Bobgy commented Nov 21, 2019

Efforts for moving frontend unit tests:

  1. Add test configuration to prow: Add kubeflow pipelines frontend unit tests to prow kubernetes/test-infra#15332, Fix kubeflow pipelines frontend test configuration kubernetes/test-infra#15349 and Update kubeflow-pipeline-frontend-test to use node:12 and adjust command kubernetes/test-infra#15358
  2. Move frontend unit tests and configure coveralls repo token to allow reporting of coverage statistics: Move frontend unit tests to prow #2637 (the token is stored in gs://ml-pipeline-test-keys/coveralls_repo_token, in prow test env, it should have access to it automatically)
  3. Change test configuration in prow to alwaysRun: true Always enable kubeflow-pipelines-frontend-test kubernetes/test-infra#15381
  4. Remove existing unit tests in .travis.yml Remove travis CI frontend tests #2647
  5. Ask repo owner to configure kubeflow/pipelines repo's branch protection to include the new test.

@cjwagner
Copy link
Member

cjwagner commented Mar 2, 2020

This is still an issue. Please address!
#3197
https://prow.k8s.io/tide-history?repo=kubeflow%2Fpipelines

@Bobgy
Copy link
Contributor

Bobgy commented Mar 3, 2020

/assign @jingzhang36
for backend tests
/assign @Ark-kun
for python tests

We will prioritize this after recent release.

@Bobgy
Copy link
Contributor

Bobgy commented Mar 11, 2020

@jingzhang36 has migrated backend test to prow: #2885

@jingzhang36
Copy link
Contributor

Backend unit tests are off Travis now (#2885 is closed).
We'll need to take care of python tests now /assign @Ark-kun

@Bobgy
Copy link
Contributor

Bobgy commented Jun 26, 2020

cla plugin lives in https://github.com/kubernetes/test-infra/tree/master/prow/plugins/cla, if we can make it configurable like which github status check should be converted to which labels, then it could potentially also be used to allow us using travis tests.

@cjwagner
Copy link
Member

IDK the details of how the google CLA is managed, but I do know that is used with Prow by the Knative org without issue. e.g. knative/serving#8487
Notice that a google-bot manages a CLA label in addition to the status context.

It seems we need a plugin similar to the existing cla plugin for cncf cla in https://prow.k8s.io/plugins.
It syncs github status to labels in the PR, so tide can set criteria on labels.

This isn't necessary. 1) It looks like google-bot can be made to manage a label. 2) Tide can be configured to require the CLA: yes label or require the CLA status context directly, but you need to configure at least one so that Tide's requirements match whatever you have configured in the GitHub settings for the repo.

@jlewi
Copy link
Contributor

jlewi commented Jun 26, 2020

We use the Google CLA on all other repos within KF and it doesn't appear to be an issue.

@Bobgy
Copy link
Contributor

Bobgy commented Jun 28, 2020

Thanks @cjwagner and @jlewi!

I found out googlebot requires the repo to already have labels named cla: yes and cla: no. I just added them and the bot is starting to add labels automatically: #4094.

@Bobgy
Copy link
Contributor

Bobgy commented Jun 28, 2020

Sent a PR kubernetes/test-infra#18095 to update kubeflow org configuration.

@Bobgy
Copy link
Contributor

Bobgy commented Jul 22, 2020

the config change had permission issues
/reopen

@k8s-ci-robot k8s-ci-robot reopened this Jul 22, 2020
@k8s-ci-robot
Copy link
Contributor

@Bobgy: Reopened this issue.

In response to this:

the config change had permission issues
/reopen

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.

@Bobgy
Copy link
Contributor

Bobgy commented Jul 22, 2020

Latest status:

k8s-ci-bot needs admin permission on kubeflow org to enforce above mentioned branch protection rule.
context: kubernetes/test-infra#18397 (comment)

ref from @jlewi:
repo permissions are granted via GitOps using periobolous so you could configure an appropriate PR.
https://github.com/kubeflow/internal-acls/blob/f6e2bdb4a92b1ae6b808f2c7b81ab1077acfa69a/github-orgs/kubeflow/org.yaml#L672

We can grant those permissions, but do we think that's safe?

@jlewi do you think kubernetes/test-infra#18397 (comment) convinced you these permissions are required and acceptable?

If not, another backup option is to add "cla: yes" label to all the repos and configure the requirement by github labels.

EDIT: sent a PR for adding cla labels anyway kubeflow/testing#741.

@jlewi
Copy link
Contributor

jlewi commented Jul 28, 2020

@Bobgy yes; I think I'm convinced by @cjwagner 's comment. @Bobgy If possible can we point at the Kubernetes test infra as prior art that they are granting the ci-bots admin permissions at the org level?

@cjwagner
Copy link
Member

FYI I am also seeing 403s from GH in the Prow logs when trying to interact with the kubeflow/blog repo. It appears that repo has revoked the permissions for the bot or never granted them. Org level admin permissions should resolve this though.

@jlewi
Copy link
Contributor

jlewi commented Aug 4, 2020

@cjwagner I think when we created kubeflow/blog we initially didn't add the ci-bots team but it should be added now; if your still seeing 403s let me know (preferablly in a new issue on kubeflow/blog).

@Bobgy I think we can also set the permissions at the org level now.

@Bobgy
Copy link
Contributor

Bobgy commented Aug 4, 2020

Okay, let me do that tomorrow

@Bobgy
Copy link
Contributor

Bobgy commented Aug 17, 2020

Fixed by kubernetes/test-infra#18837

@Bobgy Bobgy closed this as completed Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.