action #54926
closed
Re-think the github PR requirements
Added by okurz almost 5 years ago.
Updated over 4 years ago.
Description
Observation¶
- Needing to select the "Approve" action in github is annoying due to the necessity of explicitly selecting a different tab on github, selecting a button, clicking another button and only then being allowed to merge, even for trivial changes
- Branches need to be "up-to-date". There is an option to merge back changes from master into the branch before merging but this is in conflict with our previous best-practice to not allow merge commits within the branch that should be merged. Having a branch up-to-date is virtually impossible on our pretty active project os-autoinst-distri-opensuse
@szarate I am not sure if the changes that you announced today had been discussed in a broader circle so I am trying to be careful. However I disabled the setting "Require branches to be up to date before merging" again because it sounds nice but because of point 2. in the description I don't think it's a good idea. https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/8051 is the example which I hit. Please speak up if disabling this again is a bad idea and let's discuss point 1 as well. I would also be ok to give project admins more freedom as they should know better what they are doing. WDYT?
- Status changed from New to Resolved
- Assignee set to okurz
I disabled the setting "Require branches to be up to date before merging"
Thanks! I likely enabled it by accident! :) Thanks for stepping in!
I'll close this as resolved unless you want to address something else.
- Status changed from Resolved to Feedback
szarate wrote:
I'll close this as resolved unless you want to address something else.
Actually yes, the point '1.'. Would it be ok for you to enable direct merge for admins who should "know what they are doing"? We can revoke admin for everyone that shows not knowing :)
Sometimes there are cases where a change is rather obvious or the approval is implicit by e.g. a discussion in another PR. I considered https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/8091 as one of these examples. So for now I have also unchecked the "include administrators" checkbox so that admins can still override the decision to merge something regardless of check results or approval status. This still needs an explicit check of a checkbox by the person wanting to merge in red color asking if one really wants to use "admin rights" to merge. We have the same policy in place for e.g. https://github.com/os-autoinst/openQA and the team feels comfortable with that. Do you agree to keep the same policy in place?
okurz wrote:
Do you agree to keep the same policy in place?
No. But I don't care, until somebody screws up.
Would it be ok for you to enable direct merge for admins who should "know what they are doing"? We can revoke admin for everyone that shows not knowing :)
Nobody has complained about this, but you (That I know).
We can discuss this, but next time please make sure that I get the notification ok?. I happened to open this ticket again because I was looking for open tickets without brackets. Asking questions in progress on a ticket that it's assigned to yourself, will not trigger a notification for me. Or at least my email client won't show it. (The real message is: Don't expect progress to always notify people, and use direct communication)
We can discuss this, but next time please make sure that I get the notification ok?. I happened to open this ticket again because I was looking for open tickets without brackets. Asking questions in progress on a ticket that it's assigned to yourself, will not trigger a notification for me. Or at least my email client won't show it.
(The real message is: Don't expect progress to always notify people, and use direct communication)
sorry, but that might be valid for you but for others it's not the case and I was reminded by this. There are some people that state: "Please write an email, always! to everybody!", others want just IRC, others again state "ah, ok. yeah I am logged into IRC but I ignore it, please send it over Rocket.Chat (to everyone! always!). others prefer just updates in tickets. I can live with different persons having different preferences but I am simply unable to remember which persons in particular have which personal preference, I hope you can understand as well. I added you explicitly as watcher to the ticket. So either you should watch the project and receive notification for all the tickets – apparently and due to the load understandably you don't - or you should receive a notification due to being a watcher. If this does not work I can help you debug what is going wrong e.g. on progress side having access to the machine running the redmine instance.
- Subject changed from Re-think the github PR requirements to [functional][u] Re-think the github PR requirements
Let's drive this from U-Team, but involving all affected teams.
- Subject changed from [functional][u] Re-think the github PR requirements to Re-think the github PR requirements
- Status changed from Feedback to Resolved
This ticket is resolved already, from my point of view, as there was no further comments from okurz, and I don't see a point in discussing this further.
sure, it's fine because no one else commented. I am ok to use my "admin rights" in critsit and sparingly of course ;)
okurz wrote:
sure, it's fine because no one else commented. I am ok to use my "admin rights" in critsit and sparingly of course ;)
Just don't make critsit a norm ok? :)
Also available in: Atom
PDF