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
List all changes related to a build. #1305
Conversation
order_by=sa.desc(changes_tbl.c.changeid), | ||
limit=1) | ||
return conn.scalar(q) | ||
d = self.db.pool.do(thd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why create a separate variable -- d
-- if its value immediately returned?
👍 from me. I'd let others look at as well. |
return self.db.pool.do(thd) | ||
|
||
@defer.inlineCallbacks | ||
def getChanges4Build(self, buildid): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically spell out "For" in methods like this.
(Interesting, I cancel builds for e9665b0 but they still show |
td Date | ||
td Files | ||
tr(ng-repeat="change in changes | orderBy:'changeid':true") | ||
td {{ change.changeid }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's by far not limited to this commit or this place, but I don't believe ids have anything to do on the web interface. They are just internal data representation details ...
Side note: it looks like the base branch ( |
before I continue to work on this, we need on aggreement on this PR. |
I believe I'm missing something big here, as the correct fix just looks so natural to me. My point is: at some moment in time, we do have all the necessary information at hand:
The reason why I'm wondering why we are 'forgetting' this information, just in order to try to get it back via complicated ways (full commit tree in DB) later on ... To me it looks like the natural way is to write all those relations to the DB, just in order to read them later on. Please let me know what's the silly part there. |
I've been thinking about this as well, and I agree with @benallard that it is dangerous to make assumptions on build continuity, and that would be better to have more support on the core for change hierarchy. This is a tough problem which will be hard to discuss on IRC or via github. I would propose to make another hangout attempt, and try to brainstorm there, and make a decision. |
Yes, this is complicated. The bit I'd disagree with is "which change(s) lead to which sourcestamp". We know one change which led to that sourcestamp; that change represents a delta from the sourcstamp before it. With a 'parents' field for changes, we also know which changes led to that parent sourcstamp (or in case of a merge, sourcestamps), and so on. In the current arrangement, the only way that we associate multiple changes with one sourcestamp is if the scheduler aggregates them during the treeStableTimer. treeStableTimer made sense for CVS, where commits were per-file and not atomic at the repository level, but even for SVN it doesn't make sense. This arrangement misses buildrequest merging, retries, and a number of other phomena because it tries to predict -- before any builds have taken place -- the range of sourcestamps between the last known-good commit and this one. The approach I'm suggesting instead leaves that determination until the build is complete and it's time to report on its status. In the final implementation, I think that would traverse the change graph backward from the current build until it found a change for which a successful build on the same builder had been performed. This way, we use the most up-to-date information to determine who might be to blame. Fast builders which never collapse requests will always blame a single change, while slower builders which do collapse requests will have longer blamelists. This is a lot of machinery to build, and I don't think we need to build it for 0.9.0, but I feel strongly that it's the right long-term goal. |
I think these are definitely good things to think about. I know I've wanted buildbot to be a bit smarter about knowing how builds are related. But I agree that it can wait until after 0.9.0. Good things to think about though :) |
If the only missing part is indeed "which change(s) lead to which sourcestamp", then I think that this should be fixed first. I agree that the only scenario where this come in place is the treeStableTimer. But I don't think it's a thing of the past. We use it daily to collapse the commits contained in a push action ! In a multi-repository setup, you want to get the commit to the lib and the commit to the app in one build. I'm afraid I don't fully understand the goal of the 'parent' field. Is it meant to point to the sourcestamp we made before the scheduler kicks in ? As in: the parent commit ? Does this means that you aim at replicating the whole commit tree in the DB ? With support for merge, forced push, ammends, and darcs dark concepts ? I would say that this, without direct connection to the VCS tool is a lost battle. We have that old concept of "Problem" if I remember correctly, where all blame list are concatenated until the build pass again. We should aim at solving the troubles one at a time I believe, and this means first being able to give the blamelist for one particular build, based on collapsed buildrequests, and treeStableTimers's changes. |
e573884
to
862cc74
Compare
|
I'd appreciate a couple of reviewers for this PR :) |
@@ -45,6 +45,26 @@ def getBuildByNumber(self, builderid, number): | |||
(self.db.model.builds.c.builderid == builderid) | |||
& (self.db.model.builds.c.number == number)) | |||
|
|||
def getPrevSuccessfulBuild(self, builderid, number): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can return a build on any repository, on any branch. (Yes, I do have builders generic enough to be able to handle events from multiple repositories). Not necessary related to the current one !
0713b95
to
4c66990
Compare
@benallard yes, this means replicating the version-control hierarchy in the DB. Trac does this, too, actually. It's a little scary for darcs, but we'll leave that to the Darcs maintainers (maybe changes don't have parents, so you don't get a blamelist beyond one change?) Support for treeStableTimer won't go away -- it will lose the behavior of determining the set of changes "responsible for" a particular build, but will continue to determine when a scheduler produces a buildset. In this sense, it's an early, simpler version of build request collapsing (avoiding creating lots of build requests that will just be merged later). |
I think it conflicts with GH-1334 |
Yes. And I need to add more tests. I'll continue this PR next week. Sent from my iPhone
|
- to do this, the column 'parent_changeid' was added to the table changes - traverse the change graph backward from the current build until it found a change for which a successful build on the same builder had been performed. - use repository/branch/codebase of sourcestamps as a criteria to found the previous build - getParentChangeId to getParentChangeIds - parent_changeid to parent_changeids - update documentation
84c580d
to
d2a381e
Compare
rebase from master + fix conflict. Should be OK for reviewed. |
(tbl.c.results == 0)), | ||
offset=offset, | ||
limit=10) | ||
if len(prevBuilds) == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not prevBuilds:
...
?
LGTM, though I feel uneasy about really short variable names. For example, I understood Release notes are missing. |
I still expect the release notes. |
And the identation "fix" causes pep8 problems. :(
Edit: added the error but formatting sucks. |
Yes, my first commit was reindent with autopep8, and you asked about indentation. I repush indentation as it was in the first commit. |
Sorry about that. I'm really curious about this particular case. I'll look at it after it lands in master. |
The dark side of git ... |
travis is failing ... IMO, this is not related to this PR (as I did not modify any buildslave code). Also, I'm unable to reproduce the error on my desktop. maybe a race condition was introduced by #1370 ? |
They are the same as in TRAC-3066 |
(It's flaky flaky) |
List all changes related to a build.
Per comment, no release notes are required for this kind of change. |
Still, @delanne, I already mentioned the advantage of small commits to you ... So I won't repeat myself. Your main commit here is touching the db layer, the data layer, the web interface, and other bits. All in 1 commit.That's too much. |
The karma test is failing since this:
note the missing oparent_changeids in the second part. |
Just opened TRAC-3071 with an error when visiting the new data endpoint. (also posting it here to prevent it for being unseen.) |
BTW, This is a behavioral change (if I understand it correctly), as previously only the direct changes for a build were shown, now all changes since the previously sucessfull build are shown. And I can imagine that some people rather want the previous behavior. So i believe a release note update would have been required. |
Mentioning in the release notes is not a bad idea, but I think that this is the expected behavior. Of our userbase, I'd guess that 50% thought it worked this way already, 40% knew it didn't but were annoyed at the weird behavior, and 10% liked it. |
Fair enough, see #1423 and TRAC-3098 for further improvement of this feature. |
Note: