Hi,

These are interesting question to discuss.

On 07.02.18, 20:40, "James E. Blair" corvus@inaugust.com wrote:

Hi,

The problem of how to determine what has changed in the change under test has come to my attention twice today, and it's still early, so I thought it'd be a good topic for a ML discussion.

The two issues that have come to my attention represent two similar kinds of tests:

a) Inspect the specific commit(s) under test (e.g., inspect the commit message).

b) Inspect the delta of applying the patch (e.g., run coverage twice and calculate the difference).

Simon Westphahl has proposed a patch focused on the first case, which adds a zuul.commits variable with the sha(s) of the patch(es) under test: https://review.openstack.org/539975

Meanwhile, here is an example of a test which relies on the existence of an origin remote:

http://logs.openstack.org/58/473658/19/check/openstack-tox-cover/0f6849c/job...

We also have the minor example of pip emitting warnings if there is no origin remote; if we can fix that here, that's fine, but I don't think it's significant enough to drive our design.

The patch to add zuul.commits makes case (a) fairly simple -- you can have ansible iterate over the values in the variable.

I think that case (b) might be a bit harder with zuul.commits -- specialized tooling would be needed to create the alternative repo state (ie, a new branch) in order to perform the git checkout or diff operations. Because in (b) we are focused more on the previous state than the commit, we can't just checkout zuul.commit, we need to checkout the parent of zuul.commit.

However, if we were to create an origin remote, and populate it with the "previous" git repo state, (that is, either the current state of the tree, or that with the changes ahead of this one applied), it seems we can accommodate both (a) and (b) in an intuitive manner.

You could get the values that would be put in zuul.commits by performing 'git diff origin/master..master', which addresses (a), and you could address (b) with 'git checkout origin/master'.

Among the reasons we don't have any remotes on the git checkouts are:

* What would the URL be? * What would the content be? * Concern about jobs altering the git repo under test by performing naïve and incorrect git operations.

We've addressed the content question, that leaves the other two. Ideally, if we did this, I'd like to have a remote with no URL. However, while that may be possible, some investigation suggests that we may want to avoid that as it looks like it may be a bit of an edge case that not all tools or libraries would support correctly. However, if we set it to a bogus URL, that may be sufficient to satisfy the tooling, as well as prevent git repo manipulation errors.

I just tried that out and it seems while you can set an empty remote even git itself (git fetch origin) behaves differently. So if we go that way I'd suggest we use a bogus url with no network interaction like 'file:///dev/null'.

I'm inclined to favor this approach, as I think it addresses both cases and is fairly intuitive. Do folks think it would work? Are there things we would be missing as compared to the zuul.commits approach?

I think there should be no functional difference compared to what we can get via git log. However we need to cope with the merge commit zuul creates, but I think that should be no real problem. When using github we should think about how merges within the PR can be handled. In the github workflow it is very common to merge master into the PR to update it. In this case github lists the commits in an order which is not obvious at first glance. So I think in this case 'git log' might be even superior as it has some means to better filter/sort the commits compared with the commit list of github.

Thanks,

Jim

Kind regards Tobias

Clark Boylan cboylan@sapwetik.org writes:

It seems like the origin url should be the repo's canonical location url for correctness. This opens jobs up to bad git repo manipulation but nothing prevents users from doing it themselves in the current situation. If we really want to use a bogus url then avoiding "origin" might be best as the name for the remote. We could call it base or something like that.

That was my first instinct as well, however, in many cases the URL that Zuul uses or the canonical location won't be accessible to the test node, or might have a username and credentials attached. Having a working URL might make it easy to grow a dependency on that and put those kinds of systems at a disadvantage. And we should be careful not to accidentally suggest or encourage the idea that the remote connection is valid -- tests are running in a speculative future state which can not be represented outside the test environment.

Of course, some times you need to access a remote (for example, OpenStack's release and proposal jobs). I think it's okay to do extra work to set up that interaction when required, since it's really outside of the scope of Zuul.

Using the 'origin' remote has the advantage of being intuitive to most git users, and it would allow tools like pip and https://github.com/Bachmann1234/diff-cover and pip to work as designed with no effort. Diff-cover can be used with an alternate branch, so it's not a requirement, it just seems like the most sensible default.

There's no reason origin needs to be the canonical location (it is a DVCS after all), and in those cases, like the OpenStack release jobs where we do want to interact with a remote, we can make sure to update it appropriately (either by removing it or using set-url before use).

Another option may be to use a non remote ref and then you can git diff refs/zuul/base/master..master then continue to have no remotes. This avoids breaking tooling with bogus remote urls in unexpected ways but provides for jobs to find the ref that they need.

That's true. It solves the URL problem. It just makes things like diff-cover a little more difficult to use (and probably creates a difference between how a user would run diff-cover locally vs. in a test).

-Jim