search

kubeflow / pipelines

Machine Learning Pipelines for Kubeflow
https://www.kubeflow.org/docs/components/pipelines/
Apache License 2.0
3.56k stars 1.61k forks source link

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

Closed BenTheElder closed 4 years ago

BenTheElder commented 5 years ago

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 https://github.com/kubeflow/pipelines/pull/1652

cc @jlewi for kubeflow prow

cjwagner commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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

cjwagner commented 4 years ago

I also opened an issue about this in march, but I have been struggling to get anyone to address it. https://github.com/kubeflow/pipelines/issues/930

jlewi commented 4 years ago

@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 commented 4 years ago

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

IronPan commented 4 years ago

@Bobgy Thanks moving travis to prow would be great.

Bobgy commented 4 years ago

Efforts for moving frontend unit tests:

  1. [x] Add test configuration to prow: https://github.com/kubernetes/test-infra/pull/15332, https://github.com/kubernetes/test-infra/pull/15349 and https://github.com/kubernetes/test-infra/pull/15358
  2. [x] Move frontend unit tests and configure coveralls repo token to allow reporting of coverage statistics: https://github.com/kubeflow/pipelines/pull/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. [x] Change test configuration in prow to alwaysRun: true https://github.com/kubernetes/test-infra/pull/15381
  4. [x] Remove existing unit tests in .travis.yml https://github.com/kubeflow/pipelines/pull/2647
  5. [x] Ask repo owner to configure kubeflow/pipelines repo's branch protection to include the new test.
cjwagner commented 4 years ago

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

Bobgy commented 4 years ago

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

We will prioritize this after recent release.

Bobgy commented 4 years ago

@jingzhang36 has migrated backend test to prow: https://github.com/kubeflow/pipelines/issues/2885

jingzhang36 commented 4 years ago

Backend unit tests are off Travis now (https://github.com/kubeflow/pipelines/issues/2885 is closed). We'll need to take care of python tests now /assign @Ark-kun

chases2 commented 4 years ago

This issue is occurring currently, see #3876 I placed a /hold to deal with this in the short-term

Bobgy commented 4 years ago

Sorry about that @chases2, @Ark-kun said he would migrate off travis in a few days

cjwagner commented 4 years ago

Please address this. This has been negatively impacting our infrastructure for more than 15 months. https://prow.k8s.io/tide-history?repo=kubeflow%2Fpipelines&branch=master

cjwagner commented 4 years ago

Please be warned that we intend to disable Tide for this repo if the issue has not been resolved by August.

Bobgy commented 4 years ago

Hi @cjwagner, sorry for your inconvenience. Thanks for maintaining the infra!

FYI, a few weeks ago, I removed required status of Travis tests before @Ark-kun can migrate them to prow.

So I believed this issue should have been solved.

However, the issue that occured this time is different, cla requirement is not matching tide configuration. I will go ahead and fix that.

Bobgy commented 4 years ago

took a quick look, cla check seems also a github check. How does other repos use it with tide?

Can the cla bot be configured to sync with labels?

Ark-kun commented 4 years ago

@cjwagner

Please address this.

Thank you for your patience. We thought that this issue has already been fixed several weeks ago since we changed the GitHub requirements to ignore the Travis tests, so that GitHub requirements are the same as Prow. Unfortunately it looks like the Travis was not the only issue. We need every user to sign the Google CLA and we use Google's official CLA bot for that. I was under the impression that Prow understood the labels used by the Google CLA bot, but I might be mistaken.

Ark-kun commented 4 years ago

Is Prow compatible with the Google CLA?

Bobgy commented 4 years ago

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.

Bobgy commented 4 years ago

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 commented 4 years ago

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. https://github.com/knative/serving/pull/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 commented 4 years ago

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

Bobgy commented 4 years ago

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: https://github.com/kubeflow/pipelines/pull/4094.

Bobgy commented 4 years ago

Sent a PR https://github.com/kubernetes/test-infra/pull/18095 to update kubeflow org configuration.

Bobgy commented 4 years ago

the config change had permission issues /reopen

k8s-ci-robot commented 4 years ago

@Bobgy: Reopened this issue.

In response to [this](https://github.com/kubeflow/pipelines/issues/1653#issuecomment-662184872): >the config change had permission issues >/reopen Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
Bobgy commented 4 years ago

Latest status:

k8s-ci-bot needs admin permission on kubeflow org to enforce above mentioned branch protection rule. context: https://github.com/kubernetes/test-infra/issues/18397#issuecomment-662060833

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 https://github.com/kubernetes/test-infra/issues/18397#issuecomment-662066679 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 https://github.com/kubeflow/testing/pull/741.

jlewi commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Okay, let me do that tomorrow

Bobgy commented 4 years ago

Fixed by https://github.com/kubernetes/test-infra/pull/18837