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

More power for authorized users in self-service community operators #76

Closed
2uasimojo opened this issue Aug 2, 2021 · 11 comments
Closed
Assignees

Comments

@2uasimojo
Copy link
Contributor

Self-service community operator version PRs are way cool!

However, a handful of things I've noticed that would make it even cooler:

  • When adding an authorized user to ci.yaml (e.g. 1, 2), a repo admin still needs to participate to get the PR merged. It would be cool if pre-existing reviewers in ci.yaml could have that power (via /lgtm or /approve or whatever).
  • When a test fails, even an authorized (via ci.yaml) user can't seem to retest or add ok-to-test.
@mvalarh mvalarh self-assigned this Aug 9, 2021
@mvalarh
Copy link
Contributor

mvalarh commented Aug 9, 2021

* When adding an authorized user to ci.yaml (e.g. [1](https://github.com/redhat-openshift-ecosystem/community-operators-prod/pull/63), [2](https://github.com/k8s-operatorhub/community-operators/pull/65)), a repo admin still needs to participate to get the PR merged. It would be cool if pre-existing reviewers in ci.yaml could have that power (via `/lgtm` or `/approve` or whatever).

We were already allowing it, but it is not working. The problem is that when user (authorized user) would do the comment (via /lgtm or /approve or whatever) then workflow comment handler is executed with that user's permissions and if that user (authorized user) doesn't have write access to repo (by default no one has) then label cannot be set by that user. Only repo admin can. I am still thinking about this feature (not high priority). But you can give me hint if you have some proposal.

@mvalarh
Copy link
Contributor

mvalarh commented Aug 9, 2021

* When a test fails, even an authorized (via ci.yaml) user can't seem to [retest](https://github.com/k8s-operatorhub/community-operators/pull/75#issuecomment-891143786) or [add ok-to-test](https://github.com/k8s-operatorhub/community-operators/pull/75#issuecomment-891148861).

To retest one can do:

  • convert issue to draft
  • click on "Ready for review"

@mvalarh mvalarh closed this as completed Aug 9, 2021
@mvalarh
Copy link
Contributor

mvalarh commented Aug 9, 2021

Feel free to reopen

@mvalarh
Copy link
Contributor

mvalarh commented Oct 20, 2021

@2uasimojo I just implemented feature with authorized_label when PR is approved by reviewer. Let me know when you will have such PR so we can test it. Thanks

@mvalarh mvalarh reopened this Oct 20, 2021
@mvalarh
Copy link
Contributor

mvalarh commented Oct 20, 2021

Possible workflows

  1. approve
  2. /hold
  3. /unhold

or

  1. approve
  2. author will push something

or

  1. approve
  2. make draft Issue
  3. Ready for review

Note: We always rely on information that of last github reviewer and it hase to be in state APPROVED

@2uasimojo
Copy link
Contributor Author

Thanks for working on this @mvalarh!

Perhaps next Monday (when we do our regular push) we can contrive a scenario to test this. Let me know if this would work:

  1. Prepare: merge a PR removing a user from ci.yaml. It will be @abutcher's turn on rotation next week, so if he agrees, we can experiment on him :P
  2. Andrew submits the week's PR as usual. Since he's no longer in ci.yaml, it won't be approved.
  3. Have someone who's still in ci.yaml (e.g. me) approve the PR
  4. ...and IIUC at this point we have to nudge the PR somehow? Either hold->unhold, draft->ready, or Andrew pushes an update?
  5. Clean up: Restore Andrew to ci.yaml.

Will that be an effective test?

2uasimojo added a commit to 2uasimojo/community-operators that referenced this issue Oct 21, 2021
We're testing a fix whereby reviewers should be able to approve changes
by non-reviewers.

This is step 1: make abutcher not a reviewer.
Step 2: He will submit a PR.
Step 3: I will approve it. Hopefully it will merge.
Step 4: Restore abutcher to the reviewer list

Part of testing k8s-operatorhub#76
2uasimojo added a commit to 2uasimojo/community-operators that referenced this issue Oct 21, 2021
We're testing a fix whereby reviewers should be able to approve changes
by non-reviewers.

This is step 1: make abutcher not a reviewer.
Step 2: He will submit a PR.
Step 3: I will approve it. Hopefully it will merge.
Step 4: Restore abutcher to the reviewer list

Part of testing k8s-operatorhub#76

Signed-off-by: Eric Fried <efried@redhat.com>
@2uasimojo
Copy link
Contributor Author

We tested this via #324 (to remove abutcher from reviewers) and #326 (which he submitted and I approved). It works! Thanks @mvalarh!

We're going to test a different flow -- say the draft/ready one? -- by having Andrew submit the PR to restore himself to ci.yaml.

@2uasimojo
Copy link
Contributor Author

It seems like when the submitter is not a reviewer, the bot puts a /hold on the PR anyway. So the hold-unhold flow is kind of kicked off by default.

@mvalarh
Copy link
Contributor

mvalarh commented Oct 23, 2021

Bot is putting in hold only in case ci.yaml is changed

@2uasimojo
Copy link
Contributor Author

@mvalarh I'm happy with the functionality as we tested it. If you'd like us to test some other permutation, let us know. Otherwise I'm happy to close this issue.

@mvalarh
Copy link
Contributor

mvalarh commented Oct 25, 2021

@mvalarh mvalarh closed this as completed Oct 25, 2021
konrad-ohms pushed a commit to instana/community-operators that referenced this issue Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants