-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
do PR validation on Travis-CI (or GitHub Actions?) instead of Jenkins #507
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
my initial stabs at this tried to share code with the full bootstrap-and-publish job, but the bootstrap stuff is so hairy (for both inherent and historical/accidental/fixable reasons), it's going to easier to work separately from that first, then once it's working, consider removing duplication. (Lukas agrees.) the nicest experience will be achieved if we split up the parts of |
we can't publish the PR snapshots to Artifactory because pull request jobs on Travis-CI don't have access to secure environment variables I suggest we:
|
use only sbt, avoid using external shell scripts and environment variables. we just need a few simple commands right here in .travis.yml doesn't publish anything to Artifactory. let's discuss the way forward on that at scala/scala-dev#507
cleaner PR at scala/scala#6630. uses |
use only sbt, avoid using external shell scripts and environment variables. we just need a few simple commands right here in .travis.yml doesn't publish anything to Artifactory. let's discuss the way forward on that at scala/scala-dev#507
I hit merge on scala/scala#6630, so remaining todos include:
|
we haven't been super happy with Travis-CI for PR validation. timeouts, subpar UI for log browsing and analysis, ... the publishing-from-Travis aspect has worked fine, but with a big exception, which is that we don't have a way to publish Scala versions for as-yet-unmerged PRs, because we don't have access to the necessary encrypted environment variables from PR runs. (whereas our Jenkins setup has those secrets, in a way we don't think any PR can inappropriately access.) today, in 2019, GitHub Actions exists and seems worth considering as well. |
hopefully github will become a good option. recent comment by Olafur:
primarily the feature that is missing IMO is the ability to re-run an individual job that failed for some reason (flaky download, etc). Lack of caching doesn't seem to be a blocker.
|
I made a variation to run on github actions a small while back. It demonstrates running on a matrix of adoptopenjdk 8, 11 and 12 (and failing on everything but 8) https://github.com/martijnhoekstra/scala/runs/220138323 |
@smarter is pushing GitHub Actions. I think maybe he gets a commission 💰 but the Dotty folks don't have a unmerged-PR-publishing solution, scala/scala3#6145 remains open. regardless, we can surely learn from what their team is doing |
as we discussed in scala/contributors Gitter today, note that Travis-CI and Jenkins test different things. Jenkins tests the PR's branch HEAD as-is. Travis-CI tests the merge commit. (though by the time we actually hit "merge", HEAD of the target branch may already have moved on, so that result may be stale.) there are pros and cons to both. note also that with Travis-CI you get different behavior depending on whether the PR comes from a fork or not. if the PR comes from a branch in the same repo, you get both the branch run and the merged run. but in scala/scala all PRs always come from forks. so the branch run only occurs if Travis-CI has explicitly been enabled in the fork. (except, gah, at present in scala/scala that branch run will always fail; see #663) |
(Moving from scala/scala#8731 (comment))
True. I guess given enough merge conflicts, old branches will always have to be rebased before their PRs can be merged (relevant to me as I just finished rebasing scala/scala#5892). But, otherwise, the risk here is that an old branch goes green on all its commits, but it then break once merged (semantic conflicts). Makes me wonder if there's a relationship between how verbose/terse a language is and how frequent projects in those languages get merge conflicts. Did bors come out of c/c++/rust projects which had less merge conflicts but more semantic conflicts? 🤔 |
Semantic conflicts happen once in a while, recently scala/scala#8709 |
Yeah, and those are more common than my PR-of-an-old-branch case. |
Is there a list of things somewhere I can check that the new tool must cover? Wanted to try this out for GitHub Actions |
What Travis-CI currently does is in our differences I'm aware of:
in general, PR validation is easy-ish (the bootstrap aspect is only modestly tricky) and publishing is harder There would be value even in just running the tests on GitHub Actions, especially if it worked on both Linux and Windows. Even if we can't retire the rest of Jenkins yet, it would be awfully nice if we could retire the Windows aspect of Jenkins (2.13, 2.12, 2.11). (At one point we were thinking of accomplishing that by adding an Appveyor build, but if GitHub Actions can do it, then great.) If any solution can be used on all three branches (2.11.x, 2.12.x, 2.13.x), that'd be great. But there's value even in a solution that leaves out 2.11 (and/or 2.12). (For example, we didn't bother moving 2.11 release publishing off Jenkins, since there may never be another 2.11.x release.) Wholesale changes to all of this can be daunting, but incremental change can have value too. |
Dale and I got this done recently using Travis-CI's new-ish "workspaces" feature. |
Dale fixed this recently, but the tests take long enough that it's of limited usefulness to contributors, who will use up their free allotment of minutes pretty darn fast, under Travis-CI's new limits. |
closing — #751 is the new ticket on this |
I've reverted back to travis-ci.org for the rest of the month... but I'll need a new idea in the new year. |
the groundwork for this is already laid.
the main blocker had been that partest took too long but that's fixed now, so we'll be well under the 90 minute limit the Travis-CI folks have kindly granted us.
The text was updated successfully, but these errors were encountered: