replace checkin jobs with scabot logic #45
Conversation
It confuses IntelliJ
So we can reuse it to build merge commits, which are not tied to a particular PR.
TODO:
- see if this works :-)
- handle jobs without _scabot_pr param (commits built on push above),
and post commit status to appropriate commit (put branch name into pr param?)
|
preview by @SethTisue |
adriaanm
changed the title
|
|
|
(I said it confuses intellij in the commit message, btw) |
| // - PARAM_PR's value is an integer ==> assumed to be PR number, | ||
| // - otherwise, it's assumed to be the branch name of a job launched for a pushed commit (e.g., a merge or a direct push) | ||
| // TODO: report failure | ||
| case js@JobState(name, _, BuildState(number, _, _, _, _, _, _)) => |
There was a problem hiding this comment.
the nested fors and nested Futures here took me quite a while to puzzle through and figure out what was going on. would something like the following match your intent? if so, I think it reads much easier
case js @ JobState(name, _, BuildState(number, _, _, _, _, _, _)) =>
for (bs <- jenkinsApi.buildStatus(name, number)) {
log.debug(s"Build status for $name #$number: $bs")
val jobRes = JenkinsJobResult(name, bs)
val prParam = bs.parameters(PARAM_PR)
Try(prParam.toInt) match {
case Success(pr) =>
prActor(pr) ! jobRes
case Failure(_) =>
for (_ <- milestoneForBranch(prParam))
postStatus(new BaseRef(prParam), jobRes)
}
}There was a problem hiding this comment.
It's close, but would behave differently if bs.parameters(PARAM_PR) throws, which would kill the actor.
There was a problem hiding this comment.
at the end I think that should be
for {
_ <- milestoneForBranch(prParam)
_ <- postStatus(new BaseRef(prParam), jobRes)
} yield ()
There was a problem hiding this comment.
I pushed a simplification based on your suggestion.
There was a problem hiding this comment.
I liked your original suggestion better. Foreach is the right combinator here, no need to flatMap
There was a problem hiding this comment.
yeah, it still feels a bit weird to me that we're just firing up the Future and ignoring the result, without any explicit indication in the source code, but I think you're right that switching combinators isn't the right way to address it. merging.
|
comments left inline. otherwise LGTM |
replace checkin jobs with scabot logic
|
fwiw, I checked just now and this is still working — I merged a PR onto 2.11.x, and a |
[Haven't tested this yet.]
Run PR validation on pushes to master-like branches, and post status to commit.
The goal is to retire the scala-checkin jobs at EPFL (the 2.12.x is failing because no java 8)