search

istio / test-infra

Apache License 2.0
73 stars 175 forks source link

auto-merge label not set on docs cherrypicks #4140

Open craigbox opened 2 years ago

craigbox commented 2 years ago

https://github.com/istio/istio.io/#publishing-content-immediately says that when the cherrypick/release-1.14 label is set on an istio/istio.io PR, it should be merged automatically.

The label itself says "Set this label on a PR to auto-merge it to the release-1.14 branch'.

The auto-merge label isn't set on the PRs created by the cherrypicker (https://github.com/istio/test-infra/blob/master/prow/cluster/cherrypicker_deployment.yaml#L20-L29).

@ericvn assumes this is because we only want this behavior on the docs repo, and if the auto-merge label was set here, it would apply project-wide?

I can't talk for the other repos, but as the docs repo needs to cherrypick almost every change, it would be a great quality of life improvement to change this here.

ericvn commented 2 years ago

Correct, adding the label there would apply to all repos, which we don't want. I believe we just need to create another cherry-picker for the istio.io repo only which adds the additional label (and remove istio.io from the current one)..

dhawton commented 1 year ago

Is this something that is still needed?

craigbox commented 1 year ago

It would be nice, yes!

howardjohn commented 1 year ago

I am not sure it's safe to allow arbitrary people to decide arbitrary content should go to arbitrary branches without review

On Wed, Jun 28, 2023, 6:06 AM Craig Box @.***> wrote:

It would be nice, yes!

— Reply to this email directly, view it on GitHub https://github.com/istio/test-infra/issues/4140#issuecomment-1611370839, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXJZDFU7ELITK7EUPCDXNQT5DANCNFSM52FF3NHQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

howardjohn commented 1 year ago

Actually thinking about it more - this is extremely unsafe, we cannot do this without some additional restrictions.

On Wed, Jun 28, 2023, 6:10 AM John Howard @.***> wrote:

I am not sure it's safe to allow arbitrary people to decide arbitrary content should go to arbitrary branches without review

On Wed, Jun 28, 2023, 6:06 AM Craig Box @.***> wrote:

It would be nice, yes!

— Reply to this email directly, view it on GitHub https://github.com/istio/test-infra/issues/4140#issuecomment-1611370839, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXJZDFU7ELITK7EUPCDXNQT5DANCNFSM52FF3NHQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ericvn commented 1 year ago

I agree for nearly all repos, we do not want auto merge on cherry-picks for the reasons John stated. My original statement was that the behavior today is org wide, and thus we shouldn't make the change. Craig is asking about istio.io only, and for that I'm less particular about the auto-merge on the cherry-pick since it's typically doc related and for the most part, the approval is pretty automatic anyway. A reason not to do it for istio.io, would be the typical auto-merge would be into the current production branch (the istio.io website) and that's very visible very quickly after the merge.