On Fri, May 26, 2017 at 1:09 AM, James E. Blair <cor...@inaugust.com> wrote:
> Sean Dague <s...@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@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra >
_______________________________________________ OpenStack-Infra mailing list OpenStack-Infra@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra