search

thoth-station / support

ℹ Any Thoth related support questions
https://thoth-station.ninja/
0 stars 4 forks source link

Prow is requiring `/ok-to-test` on bot accounts even though `ok-to-test` label is already present #122

Closed tumido closed 1 year ago

tumido commented 3 years ago

Describe the bug We use dependabot to manage NPM package upgrades and dependencies. Dependabot doesn't have a real github user account that can be invited to the org. And prow requires all external contributions to be approved to be tested. I've configured dependabot to apply the ok-to-test label on PR creation but that doesn't help and prow still applies the need-ok-to-test label even when there's ok-to-test label present already.

To Reproduce Steps to reproduce the behavior:

  1. Install dependabot
  2. Configure it so it applies ok-to-test label
  3. Let it create PR
  4. Let the PR be noticed by Prow
  5. See that the PR now has both ok-to-test and needs-ok-to-test labels

Expected behavior When ok-to-test is present the needs-ok-to-test is not applied and tests are run without Prow whining about it.

Screenshots image

Additional context See this PR as an example: https://github.com/AICoE/meteor/pull/185

And this dependabot config: https://github.com/AICoE/meteor/blob/main/.github/dependabot.yml

sesheta commented 3 years ago

@tumido: This issue is currently awaiting triage. One of the @thoth-station/devsops will take care of the issue, and will accept the issue by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.
tumido commented 3 years ago

/cc @harshad16

goern commented 3 years ago

sounds to me like the trigger plugin is not recognizing dependabot as an org member/collaborator. this is the root cause, not if the label is present or not. see https://github.com/kubernetes/test-infra/blob/b0788a9d385fbb8c9d8b4aab16dd1633e1bee7e1/prow/plugins/config.go#L426-L428

/kind question /priority importnat-longterm /assign @codificat /triage accepted

sesheta commented 3 years ago

@goern: The label(s) priority/importnat-longterm cannot be applied, because the repository doesn't have them.

In response to [this](https://github.com/thoth-station/support/issues/122#issuecomment-947528323): >sounds to me like the trigger plugin is not recognizing dependabot as an org member/collaborator. this is the root cause, not if the label is present or not. see https://github.com/kubernetes/test-infra/blob/b0788a9d385fbb8c9d8b4aab16dd1633e1bee7e1/prow/plugins/config.go#L426-L428 > >/kind question >/priority importnat-longterm >/assign @codificat >/triage accepted 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.
goern commented 3 years ago

/priority important-longterm

tumido commented 3 years ago

Yes.. the problem with org members/colaborators is that you have no way to invite dependabot to the org, since it's an app account, not a user account... :shrug:

codificat commented 2 years ago

As ok-to-test implies trust, do we want to have this automated to begin with?

Dependabot is not the only affected bot. Here is an example with sefkhet-abwy: https://github.com/thoth-station/report-processing/pull/145

There seem to be explicit hardcoded exceptions for our bots: https://github.com/AICoE/sefkhet-abwy/blob/1fa87f71c4fe01597fbbd8e553d0481b15db1561/aicoe/sesheta/review_manager.py#L168-L190

Just double checking here: do we want something similar for dependabot?

tumido commented 2 years ago

As ok-to-test implies trust, do we want to have this automated to begin with?

@codificat Of course we do! Using any bots implies trust. So.. if user voluntarily opts-in to use a bot to maintain their dependency stack - like dependabot in this case and even configures it to apply the ok-to-test label because he knows the bots purpose and wants the PRs validated by CI. If all of this is true, I don't think Prow should come and block it (by applying neees-ok-to-test label).

I see few problems here:

  1. Prow doesn't allow us to say that we trust any github appication with PRs. Using the same narrative Prow automatically assumes we trust all organization members, because org members are automatically "ok-to-test" every time. If we have to always trust all the org members with running the CI on their PRs, I think we should have the option to say we trust a bot with the same.
  2. Prow is happy with PRs that comes with ok-to-test and still applies the needs-ok-to-test label. PRs with both labels should simply not be possible and if the PR already has the ok-to-test label when Prow is checking it, Prow should not be stubborn about applying the needs-ok-to-test.
  3. If I opt in to use a bot to make PRs for me, I really don't want to come to all the PRs it creates and every time see that the CI was not executed, because Prow decided so. A review on a simple dependency bump PR is now turned from a minute task when everything is already laid out in front of me into a process when I need to permit the CI, wait for it and if it fails I'd need to come back to the PR... It costs time
  4. From experience - I have /ok-to-tested all the PRs created by dependabot. Every time. I think I can say I trust it with CI.
  5. Bots should relieve us from doing chores, not create new chores. This is a chore for me.

There seem to be explicit hardcoded exceptions for our bots: https://github.com/AICoE/sefkhet-abwy/blob/1fa87f71c4fe01597fbbd8e553d0481b15db1561/aicoe/sesheta/review_manager.py#L168-L190

I don't think this is related. The problem we're discussing here is that we've already applied the ok-to-test label on the PR. We should have already bypassed it. Yet Prow is still stubborn about it and adds the needs-ok-to-test and PR is then left with both of the labels meaning CI is not executed. Also we're not talking here about auto-approve, just ok-to-test.

Just double checking here: do we want something similar for dependabot?

Ideally Prow should allow us to configure the "external users" that are not required to be OKed to test, similarly as it does with branch protection

sesheta commented 2 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

sesheta commented 2 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

sesheta commented 2 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

/close

sesheta commented 2 years ago

@sesheta: Closing this issue.

In response to [this](https://github.com/thoth-station/support/issues/122#issuecomment-1115314567): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >/close 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.
tumido commented 2 years ago

/reopen This should be solved by upgrading Prow, see: https://github.com/kubernetes/test-infra/pull/24978

There's a new option to use in plugins.yaml

triggers:
- repos:
  - ...
  trusted_apps:
  - dependabot
  - renovate
  - ...
sesheta commented 2 years ago

@tumido: Reopened this issue.

In response to [this](https://github.com/thoth-station/support/issues/122#issuecomment-1117372233): >/reopen >This should be solved by upgrading Prow, see: https://github.com/kubernetes/test-infra/pull/24978 > > >There's a new option to use in `plugins.yaml` > >``` >triggers: >- repos: > - ... > trusted_apps: > - dependabot > - renovate > - ... >``` 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.
harshad16 commented 2 years ago

/sig devsecops /lifecycle frozen

tumido commented 1 year ago

/close

Resolved. As tested by https://github.com/operate-first/apps/pull/2611 trusted_apps work

sesheta commented 1 year ago

@tumido: Closing this issue.

In response to [this](https://github.com/thoth-station/support/issues/122#issuecomment-1317376116): >/close > >Resolved. As tested by https://github.com/operate-first/apps/pull/2611 `trusted_apps` work 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.