[ci] skip doc jobs take #3

[stas00]

@LysandreJik found another edge case when a developer force-pushes a change and pipeline.git.base_revision is defined but bogus, resulting in a range that returns no files. #8853 (comment)

So the proposed logic for take 3 is:

1. if pipeline.git.base_revision and pipeline.git.revision are defined
2. if git diff --name-only range returns anything
3. if what it returned in 2 is just docs
4. then skip

Bottom line, we skip the test altogether if:

unless test -n "<< pipeline.git.base_revision >>" && test -n "<< pipeline.git.revision >>" \
&& test -n "$(git diff --name-only << pipeline.git.base_revision >>...<< pipeline.git.revision >>)"

@LysandreJik, @sgugger

[stas00]

There has been no traction so far on the circleci forums, I filed a support ticket with cirlceci.

[sgugger]

LGTM!

[LysandreJik]

LGTM! Let's monitor.

[stas00]

So pipeline.git.base_revision is consistently undefined when making a PR via direct file edit on github.

[stas00]

So far so good.

And while monitoring I discovered an interesting thing. In this particular PR my check doesn't actually do what I thought it did. It doesn't check the range of commits from the beginning of PR. The range it checks is actually just for the last commit. That pipeline.git.base_revision is very unruly.

You can see a good example of it here: #8918

If you look at the checks for the last few commits which are doc-only commits - the jobs are skipped, whereas any commit that had code in it is not skipped.

So actually this is better than what I intended. Since if we check the full range and there are code files and then there is a subsequent commit that has only docs changed in my vision it'd run the jobs normally. But this is better! Since this checks each commit and decides whether to run the jobs or not based on just that commit - this is much more efficient than my original intention.

I hope I explained it clearly.

edit Hmm, but what happens if several commits are pushed at once - which file range will it check - since normally it checks just the last commit - this I'm not sure about. pipeline.git.base_revision is a wild card it seems.

[sgugger]

Mmmm, that does mean that if a PR changes code then has a last commit that only changes the doc, it will appear green to us, correct?
If so, we should fine a way to correct this behavior as it will lull us (and the user) in a false sense that everything is alright.

[stas00]

I will run tests once github works again and adjust accordingly.

I'm also in touch with an engineer at circleCI via their support - so hopefully we will get some solid answers rather than needing to validate all the different circumstances.

[stas00]

I wasn't able to reproduce it, but it's very clear that it happened, and this is not what we want.

And while what I wrote here #8885 (comment) is super-cool, it can't work since github relies on the last check for the overall status. So, we can only skip a job if all files in PR were docs.

So I merged a change which disabled that struggling new feature, but added a log instead to continue monitoring it while waiting for circleCI support to get back to me.