Open Stack

On Fri, May 26, 2017 at 1:09 AM, James E. Blair <corvus at inaugust.com> wrote:

> Sean Dague <sean at dague.net> writes:
>
> > On 05/24/2017 07:04 PM, James E. Blair wrote:
> > <snip>
> >> The natural way to identify a GitHub pull request is with its URL.
> >>
> >> This can be used to identify Gerrit changes as well, and will likely be
> >> well supported by other systems.  Therefore, I propose we support URLs
> >> as the content of the Depends-On footers for all systems.  E.g.:
> >>
> >>   Depends-On: https://review.openstack.org/12345
> >>   Depends-On: https://github.com/ansible/ansible/pull/12345
> >>
> >> Similarly to the Gerrit change IDs, these identifiers are easily
> >> navigable within Gerrit (and Gertty), so that reviewers can traverse the
> >> dependency chain easily.
> >
> > Sounds sensible to me. The only thing I ask is that we get a good clock
> > countdown on when it will be removed. Upgrade testing is one of the
> > places where the multi branch magic was really useful, so it will take a
> > little while to get good at it.
>
> Yes!
>
> > For gerrit reviews it should also accept -
> > https://review.openstack.org/#/c/467243/ (as that's what is in people's
> > browser url bar).
>
> Yeah, I was thinking of copying Gertty's URL parsing here which deals
> with all the variants.
>
> This reminds me of something I forgot to mention: we should *not* depend
> on specific patchsets even if the URL specifies it.  Sometimes you end
> up with:
>
>   https://review.openstack.org/#/c/467634/1
>
> as the URL, with the patchset at the end.  I think that still confuses a
> lot of people and they don't notice.  And generally, if someone is
> specifying a dependency, they mean the change in general, and don't want
> to have to go update the depending change's commit message if they fix a
> typo.
>
> So I think that we should start out by simply silently ignoring any
> patchset elements in the URL.  We could consider having Zuul leave a
> note indicating that the patchset component is not necessary and is
> being ignored.
>


Hmm, I'm not sure if that's the best way to handle it. If somebody clicks
the link they'll be shown a particular patchset (whether they are aware of
not) and it may cause confusion if something else is applied in testing.
zuul leaving a message could help clarify this, but perhaps we should
honour the patchset in the URL to allow for some very specific testing or
re-runs. This also links into what I was saying (in my now forwarded
message) about "tested-with" vs "must be merged first". We could test with
a patchset but that is irrelevant once something has merged (unless we add
complexity such as detecting if the provided patchset version has merged or
if it was a different one and therefore the dependency isn't met and needs
updating).

Either way I like the idea of zuul (or something) leave a message to be
explicit.


>
> > And while this change is taking place, it would be nice if there was the
> > ability to have words after the url. I've often wanted:
> >
> > Depends-On: https://review.openstack.org/12345 - nova
> > Depends-On: https://review.openstack.org/12346 - python-neutronclient
> >
> > Just as a quick way to remember, without having to link follow, which of
> > multiple depends on are which projects. I've resorted to putting them on
> > top, but for short things would love to have them on the same line.
>
> That seems reasonable.  In a reply to Jeremy and Tristan, I suggested we
> may want to extend the Depends-On syntax in the future to consume some
> more information after the URL, but I think it should be fine to allow
> arbitrary text now and then reclaim keywords (like "applied to") later
> if necessary.
>
> -Jim
>
> _______________________________________________
> OpenStack-Infra mailing list
> OpenStack-Infra at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-infra/attachments/20170526/f8ff93e9/attachment.html>