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

Don't require /packit build by user with write permissions #920

Closed
praiskup opened this issue Jan 11, 2021 · 12 comments · Fixed by #936
Closed

Don't require /packit build by user with write permissions #920

praiskup opened this issue Jan 11, 2021 · 12 comments · Fixed by #936
Assignees
Labels
area/user-experience Usability issue

Comments

@praiskup
Copy link

Since Packit creates separate projects for each PR IIUC, it is not necessary
to - at least from Copr POV - require maintainers to trigger the build. We
can simply start them.

@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Jan 11, 2021

TBH, I am not 100% sure if I get this right.

Since Packit creates separate projects for each PR

Yes, by-default packit is the owner of that Copr project.

it is not necessary to - at least from Copr POV - require maintainers to trigger the build.

You mean GitHub project maintainer, don't you?

If the user or organisation is approved (and it is I think) and user has merge permission in that project, we trigger the build automatically. (I think the write access is the one we are speaking about, maintain is higher than write.)

If this does not work, there might be some bug during the process and we should fix it.

@TomasTomecek TomasTomecek added the area/user-experience Usability issue label Jan 11, 2021
@TomasTomecek
Copy link
Member

this would make things so much easier on our side

@praiskup if you are fine with such behaviour, as the COPR project lead, I'm pretty sure we can deliver :)

@praiskup praiskup changed the title Don't require /packit build by real maintainer Don't require /packit build by user with write permissions Jan 11, 2021
@praiskup
Copy link
Author

I think the write access is the one we are speaking about, maintain is higher than write

That's what I meant ... from Copr perspective, you don't need to require write
permissions as the users can not make any damage. The only thing that user
can affect is the build itself, or crack the concrete builder VM. The VMs are
reused for other builds, but only for buidls in the same project (and as each
PR means separate Copr project, it is just fine).

this would make things so much easier on our side

Packit would be actually even more convenient to use.

praiskup added a commit to praiskup/tito that referenced this issue Jan 11, 2021
Enable it on all currently supported Fedora/EPEL architectures.

Packit doesn't support our way of preparing source RPMs via the command
'tito build --srpm --test', but creates it's own source RPM instead.
This doesn't seem to cause a practical problems, so we are good to go
for now till the Packit RFE is implemented:
packit/packit-service#920

Fixes: rpm-software-management#395
@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Jan 11, 2021

@praiskup Thanks for the explanation.

Can we get rid of the whitelist as well then? (I think you discussed this with @TomasTomecek some time ago.)

@praiskup
Copy link
Author

I'm not sure what do you mean by whitelist in this context.

@lachmanfrantisek
Copy link
Member

OK, we have two-step verification:

  1. Allow namespace/user to build in Copr in general (=whitelist).
  2. Allow PR author to build in that specific project (required write permission in that project).

@praiskup
Copy link
Author

  1. Allow namespace/user to build in Copr in general (=whitelist).

Ah, from Copr perspective you (packit user) are the user who is allowed
to create projects and submit builds. So the problem like who/what is able
to build in Copr through GH pull request is solely in your hands.

What I mean..., yes - there are some Copr rules which packit user needs to follow,
like legal POV and others, even packit docs say that. But if someone crosses the border through PR, I'm sure it
would be logged somewhere and packit moderators can then administratively
remove such affected build, etc.

@praiskup
Copy link
Author

Can we get rid of the whitelist as well then?

I'm reading this question once more, and now I'm sure it was not targeted to me :-) sorry for noise.

@TomasTomecek
Copy link
Member

Can we get rid of the whitelist as well then?

I'm reading this question once more, and now I'm sure it was not targeted to me :-) sorry for noise.

No worries, thanks for the great elaboration.

Can we get rid of the whitelist as well then? (I think you discussed this with @TomasTomecek some time ago.)

I'd say we can't get rid of it for good. Though we could create a better solution which would be more straightforward (e.g. after installing packit, we'd redirect to our page where we would provide "rules" of the service which our users would need to "accept").

praiskup added a commit to praiskup/tito that referenced this issue Jan 12, 2021
Enable it on all currently supported Fedora/EPEL architectures.

Packit doesn't support our way of preparing source RPMs via the command
'tito build --srpm --test', but creates it's own source RPM instead.
This doesn't seem to cause a practical problems, so we are good to go
for now till the Packit RFE is implemented:
packit/packit-service#920

Fixes: rpm-software-management#395
FrostyX pushed a commit to rpm-software-management/tito that referenced this issue Jan 12, 2021
Enable it on all currently supported Fedora/EPEL architectures.

Packit doesn't support our way of preparing source RPMs via the command
'tito build --srpm --test', but creates it's own source RPM instead.
This doesn't seem to cause a practical problems, so we are good to go
for now till the Packit RFE is implemented:
packit/packit-service#920

Fixes: #395
@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Jan 19, 2021

To clarify what we want to do:

  • Do not require write permission for pull-request authors to trigger the build. (The task of this issue.)
  • Still require some validation of the project owners.

Updating the Packit upstream work column so we can groom it and start the work.

mfocko added a commit to mfocko/packit-service that referenced this issue Jan 28, 2021
Disables checking of user's write permissions in the repository that PR
is created against.

Fixes packit#920

Signed-off-by: Matej Focko <mfocko@redhat.com>
@mfocko
Copy link
Member

mfocko commented Jan 28, 2021

I'd like to clarify the outcome of this card:

  1. based on the title and description of the issue, we want to allow anyone creating PR against a repository to trigger the build (change included in the PR)
  2. do we also want to apply similar set of rules to comments (e.g. /packit build)?
    (change in PR allows only initial build after creating the PR, not comments by author of the PR without write permissions)
    a. keep as is
    b. allow author of the PR to trigger the build by commenting too (ignoring if author has or has not the write permissions to the repository)

@TomasTomecek
Copy link
Member

I'd like to clarify the outcome of this card:

  1. based on the title and description of the issue, we want to allow anyone creating PR against a repository to trigger the build (change included in the PR)

+1

  1. do we also want to apply similar set of rules to comments (e.g. /packit build)?
    (change in PR allows only initial build after creating the PR, not comments by author of the PR without write permissions)
    a. keep as is
    b. allow author of the PR to trigger the build by commenting too (ignoring if author has or has not the write permissions to the repository)

I'd say that we should allow PR authors to do /packit build: on the other hand, if some random Joe Foo Bar comments on a PR where he's not the author, we should not change commit statuses and probably comment in that PR that the user doesn't have permissions to do that.

mfocko added a commit to mfocko/packit-service that referenced this issue Jan 29, 2021
Disables checking of user's write permissions in the repository that PR
is created against.

Fixes packit#920

Signed-off-by: Matej Focko <mfocko@redhat.com>
mfocko added a commit to mfocko/packit-service that referenced this issue Feb 1, 2021
Disables checking of user's write permissions in the repository that PR
is created against.

Fixes packit#920

Signed-off-by: Matej Focko <mfocko@redhat.com>
mfocko added a commit to mfocko/packit-service that referenced this issue Feb 3, 2021
Disables checking of user's write permissions in the repository that PR
is created against.

Fixes packit#920

Signed-off-by: Matej Focko <mfocko@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Usability issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants