Build properties data #1387
Build properties data #1387
Conversation
|
I kindly disagree regarding release notes: some people come to see the status of the nine based on what's documented in the release notes, and a new data endpoint is worth mentioned, IMHO. I'm inclined to change that big TODO item to something like: "when preparing a 0.9.0 release summarise some of the changes below". |
|
The point is: That TODO is a perfect excuse (let's call a cat a cat) for me not to document it. that's an exact demonstration of the broken window theory, or why todo shouldn't exists, or should start throwing exceptions after an expiration date (Idea stolen from a blog post) ... I don't feel like documenting nine in the release note, but I can add a line about the new endpoint. I'm not sure where (as the data API is not mentioned there yet ...). For further improvements, I would suggest replacing the todo, with a (very) high-priority TRAC issue. |
|
The TODO is meant to be replaced with a paragraph summarizing the changes, rather than anything detailed. We haven't added release notes for other specific data API features or endpoints either - from the perspective of someone upgrading from 0.8.9, all of the data API is a new feature. In the early bits of nine we tracked its status in a bullet-pointed README.md, but that has since been moved to Trac. I'll remove the TODO, but this particular PR doesn't require a relnote. |
| fakedb.Buildslave(id=42, name="Friday"), | ||
| fakedb.Build(id=786, buildrequestid=5, masterid=3, buildslaveid=42), | ||
| fakedb.BuildProperty(buildid=786, name="year", value=1651, source="Wikipedia"), | ||
| fakedb.BuildProperty(buildid=786, name="island_name", value="despair", source="Book"), |
|
This looks fine to me aside from those really tiny comments. I'd like one more set of eyes, though. |
|
👍 |
benallard
force-pushed
the
build_properties_data
branch
from
cea662d
to
ce0c8f5
Compare
|
Rebased + doc fixed. |
| rv = (1, 2) | ||
| m = mock.Mock(return_value=defer.succeed(rv)) | ||
| # XXX: Does this really belongs here ? (``db.builds``) | ||
| setattr(self.master.db.builds, dbMethodName, m) |
There was a problem hiding this comment.
I dont understand the XXX, not sure if you want to keep it.
There was a problem hiding this comment.
Oh, sure ! This aims at pointing out that this data_properies stuff cross references db_builds (instead of db_properties which doesn't exists), just to prevent surprises ...
|
LGTM! |
|
@jaredgrubb, thanks, but that one was merged this morning already ; The one in #1380 still needs some eyes though. |
This is the second part of #1380 (after #1384). A third one is expected.
This relies on #1386 (first two commits of this PR), so please don't merge before the other one gets in. This makes the diff a bit bigger (7 lines ...).
There is no release note as ... well, data API change are not changes as such as they are (as a whole), part of the the big TODO on top of the release note
talk about big new Nine features.It was a suggestion from @tardyp to move the update method to the properties.py file, which makes the test a build awkward as the db stuff in on the build object ...
The
do_test_callthroughis duplicated at multiple places, and doesn't really work as expected (argsget unexpected content), this should be part of another PR.Hope you enjoy it !