Make Travis clone the full repo and the exact commit requested#8331
Make Travis clone the full repo and the exact commit requested#8331edoakes merged 3 commits intoray-project:masterfrom
Conversation
|
Can one of the admins verify this patch? |
| git: | ||
| clone: false # Clone manually to work around Travis issues like https://github.com/travis-ci/travis-ci/issues/6337 | ||
| depth: false # Shallow clones can prevent diff against base branch | ||
| quiet: true |
There was a problem hiding this comment.
(clone: false renders the other lines moot, but I kept them both for documentation and so they have the proper behavior in case someone changes clone: false later.)
|
Test PASSed. |
|
Test PASSed. |
|
Cloning the exact commit makes a lot of sense. Should we even be running the CI job if the latest commit isn't the one in the build, though? Also not totally sure I understand the shallow clone issue. Is the problem that sometimes the PR creator's branch is >50 commits behind master? |
|
@edoakes I'm not sure, but it shouldn't really affect this. Note that right now, if none of the jobs for that build have started, then they already get canceled on Travis anyway, so I guess the answer is a qualified "no". But if one of them has started, then the current behavior is that the other jobs end up building the wrong (latest) commit redundantly, which is definitely actively harmful. It seems strictly better to at least be consistent about the commit being built, which is what this PR is for. The question of whether we want to be more proactive about canceling things is definitely a valid one we should discuss but orthogonal to this PR I think. (In fact, I had a PR to auto-cancel in-progress builds upon a push, but it was rejected.) Regarding the shallow clone, I believe that's one situation where it currently happens, yeah. I'm not entirely clear if it's the only situation though (it might even depend on whether we're cloning the latest commit or the queued commit). But for example, it occurred here recently, which appeared to be because master was so far ahead that the diff failed to find the commit on it. |
edoakes
left a comment
There was a problem hiding this comment.
@mehrdadn LGTM but the lint build is failing:
https://travis-ci.com/github/ray-project/ray/jobs/328422594
|
@edoakes Oh wow it looked unrelated at first glance, I didn't realize it might be due to this. Thanks for catching! I'll see if I can fix it. |
|
Test PASSed. |
|
Test PASSed. |
…roject#8331) Co-authored-by: Mehrdad <noreply@github.com>
Why are these changes needed?
Travis only clones with a shallow depth, preventing diffs from succeeding when the commit is too old, thus causing such old jobs to fail.
Travis always clones the last commit on a PR, not the one the job is actually triggered for. This can be very misleading and confusing, as there can be several hours between job queuing and cloning.
Checks
scripts/format.shto lint the changes in this PR.