Re: [OpenStack-Infra] Zuul V3: Behavior of change related requirements on push like events

[Clark Boylan]

On Tue, May 30, 2017, at 01:45 PM, James E. Blair wrote:
> Jeremy Stanley  writes:
> 
> > On 2017-05-30 12:53:15 -0700 (-0700), Jesse Keating wrote:
[...]
> >> Personally, my opinions are that to avoid confusion, change type
> >> requirements should always fail on push type events. This means
> >> open, current-patchset, approvals, reviews, labels, and maybe
> >> status requirements would all fail to match a pipeline for a push
> >> type event. It's the least ambiguous, and promotes the practice of
> >> creating a separate pipeline for push like events from change like
> >> events. I welcome other opinions!
> >
> > This seems like a reasonable conclusion to me.
> 
> Agreed -- we haven't run into this condition because our pipelines are
> naturally segregated into change or ref related workflows.  I think
> that's probably going to be the case for most folks, so codifying this
> seems reasonable.  However, I could simply be failing to imagine a
> pipeline that works with both.

+1. I want to say that Gozer ran into some weird behavior at one point
where they were trying to enforce change details on a ref-updated
pipeline and that caused problems. The biggest issue was lack of clear
logging of the issue. We should try to clearly present this mismatch of
info to the user through the logs at the very least to avoid that.

Clark

___
OpenStack-Infra mailing list
OpenStack-Infra@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra

Re: [OpenStack-Infra] Zuul V3: Behavior of change related requirements on push like events

[James E. Blair]

Jeremy Stanley  writes:

> On 2017-05-30 12:53:15 -0700 (-0700), Jesse Keating wrote:
> [...]
>> Github labels: This is like approvals/reviews.
> [...]
>
> Perhaps an interesting aside, Gerrit uses the same term (labels) for
> how we're doing approvals and review voting.

Yeah, or at least, related.  I think in Gerrit a "label" is a review
category (eg, "Verified", "Code Review") and an "approval" is a value
given by a user to a change in one of those categories (eg, "Verified:
+1", or "Code Review -2" would be an interestingly named "approval").

Of course, that's new[1]; they used to be called "categories" rather
than "labels".

>> Personally, my opinions are that to avoid confusion, change type
>> requirements should always fail on push type events. This means
>> open, current-patchset, approvals, reviews, labels, and maybe
>> status requirements would all fail to match a pipeline for a push
>> type event. It's the least ambiguous, and promotes the practice of
>> creating a separate pipeline for push like events from change like
>> events. I welcome other opinions!
>
> This seems like a reasonable conclusion to me.

Agreed -- we haven't run into this condition because our pipelines are
naturally segregated into change or ref related workflows.  I think
that's probably going to be the case for most folks, so codifying this
seems reasonable.  However, I could simply be failing to imagine a
pipeline that works with both.

-Jim

[1] As of six years ago.

___
OpenStack-Infra mailing list
OpenStack-Infra@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra

Re: [OpenStack-Infra] Zuul V3: Behavior of change related requirements on push like events

[Jeremy Stanley]

On 2017-05-30 12:53:15 -0700 (-0700), Jesse Keating wrote:
[...]
> Github labels: This is like approvals/reviews.
[...]

Perhaps an interesting aside, Gerrit uses the same term (labels) for
how we're doing approvals and review voting.

> Personally, my opinions are that to avoid confusion, change type
> requirements should always fail on push type events. This means
> open, current-patchset, approvals, reviews, labels, and maybe
> status requirements would all fail to match a pipeline for a push
> type event. It's the least ambiguous, and promotes the practice of
> creating a separate pipeline for push like events from change like
> events. I welcome other opinions!

This seems like a reasonable conclusion to me.
-- 
Jeremy Stanley

___
OpenStack-Infra mailing list
OpenStack-Infra@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra

[OpenStack-Infra] Zuul V3: Behavior of change related requirements on push like events

[Jesse Keating]

As I was building up the GitHub driver, I noticed that in both Gerrit and
Github drivers we have some pipeline and trigger requirements that are
specific to a proposed change: A Gerrit Change or a Github Pull Request.
These types of requirements include open, current-patchset, and approvals.
In GitHub land, this includes reviews, and could (in the future) include
labels too. There is also commit status in Github land, which is actually
attached to the commit itself, rather than the pull request, so commit
status _could_ be used with push events

That said, there are event types in both gerrit and GitHub that occur
outside of a Change. In Gerrit, ref-updated events happen when code is
pushed or deleted (or merged?) to a branch (or a tag is added/removed?).
Github has a similar event type, a "push" event.

The trouble occurs when a pipeline is configured to have a requirement that
is "change type" specific, but the trigger is a push type event. When Zuul
checks to see if the requirements are met there are tracebacks (at least in
the case of current-patchset, I haven't tested the others yet). This is
happening because the push type events create a Ref object, while the
change type events create a Change object, and they have different base
attributes. A Ref object does not include the attributes necessary to
compute "current-patchset" data.

Now, I can easily patch around this to avoid the tracebacks, but it would
require defining some behavior, and that should get some thought by a wider
group of folks than me, myself, and I.

open: This could either always be true, or always be false.

current-patchset: This could always be true for push type events, or we
could examine the target ref and make judgment as to if the hash in the
event is still the "current" hash of the target.

approvals / reviews: Since there is no change associated with the push,
there can't really be any approvals or reviews. Requiring an approval and
getting a push event is... weird. Should we just pass push events through,
ignoring required approvals? Or always block them, since there can't ever
be an approval?

Github labels: This is like approvals/reviews. There is no pull request,
there cannot be a label. We either need to always pass or fail this
requirement.

Github status: Currently we only look at status for Change based events,
but this could be extended to look at them on Ref based events too, but in
a reasonably responsive Zuul set up the status on a commit may not be set
by the time Zuul checks it for scheduling purposes (then again, the
pipeline could be triggered BY the status being set, which brings up
another hairball, where we're only mapping status events back to an open
pull requests, we'd have to change all that around).

Personally, my opinions are that to avoid confusion, change type
requirements should always fail on push type events. This means open,
current-patchset, approvals, reviews, labels, and maybe status requirements
would all fail to match a pipeline for a push type event. It's the least
ambiguous, and promotes the practice of creating a separate pipeline for
push like events from change like events. I welcome other opinions!

- jlk
___
OpenStack-Infra mailing list
OpenStack-Infra@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra