Merge lp:~james-w/launchpad/expose-blueprints into lp:launchpad
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Merged at revision: | 11997 | ||||
| Proposed branch: | lp:~james-w/launchpad/expose-blueprints | ||||
| Merge into: | lp:launchpad | ||||
| Diff against target: |
1214 lines (+857/-115) 9 files modified
lib/canonical/launchpad/doc/tales.txt (+6/-2) lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+12/-0) lib/lp/blueprints/interfaces/specification.py (+129/-96) lib/lp/blueprints/interfaces/specificationtarget.py (+19/-0) lib/lp/blueprints/model/specification.py (+24/-5) lib/lp/blueprints/stories/standalone/sprint-links.txt (+0/-4) lib/lp/blueprints/tests/test_implements.py (+61/-0) lib/lp/blueprints/tests/test_webservice.py (+550/-0) lib/lp/testing/factory.py (+56/-8) |
||||
| To merge this branch: | bzr merge lp:~james-w/launchpad/expose-blueprints | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Curtis Hovey (community) | curtis | Needs Fixing on 2010-07-16 | |
| Leonard Richardson (community) | 2010-07-15 | Needs Information on 2010-07-16 | |
|
Review via email:
|
|||
Commit Message
Blueprints now have a basic API exposed, as "specifications".
Description of the Change
Hi,
Summary
Expose some blueprint attributes and a couple of methods over the API.
Proposed fix
Add exported() in a bunch of places.
Pre-implementation notes
None.
Implementation details
It exports the basics of ISpecification, plus 3 methods that may be useful
to people for getting blueprints.
I added two new methods to IHasSpecifications, as if we exposed the existing
attributes we would serve the collections when just getting the specification.
I'm not entirely sure what getValidSpecifi
inconsistent implementations, so perhaps it is not worth exposing it.
A getSpecifications() similar to searchTasks() would be great, but the API isn't
there for that.
Also, I guess there may be opposition to exposing the whiteboard, but I think
if it is there then it should be exposed, and as it will be one of the most
used attributes not having it would make the API close to useless, at least for
removing the screen scraping in launchpad-
Tests
./bin/test -s lp.blueprints.tests -m test_webservice
Demo and Q/A
I'll test it with launchpadlib once it is on edge.
lint
./lib/lp/
336: E222 multiple spaces after operator
375: E222 multiple spaces after operator
681: E231 missing whitespace after ','
./lib/lp/
12: source has bad indentation.
19: source has bad indentation.
32: source has bad indentation.
41: source has bad indentation.
65: source exceeds 78 characters.
65: source has bad indentation.
81: source has bad indentation.
86: source has bad indentation.
98: source has bad indentation.
105: source has bad indentation.
./lib/lp/
824: E231 missing whitespace after ','
1823: E231 missing whitespace after ','
1946: E301 expected 1 blank line, found 0
None of which I am inclined to fix, as they are not in code that I have
changed or would require updating too much of the file to be worthwhile.
| James Westby (james-w) wrote : | # |
On Fri, 16 Jul 2010 13:05:52 -0000, Leonard Richardson <email address hidden> wrote:
> Review: Needs Information
> Thanks a lot for this branch. This is a branch I was planning to create as a way of introducing Benji to the web service, but instead we went through your branch and reviewed it.
>
> I don't have any opinion on the whiteboard thing, but I'll make sure someone with an opinion looks at this branch.
>
> = Questions about the exported objects =
>
> 1. Obviously you don't need to export everything to make this branch
> useful, but I wonder why you exported ISpecification.
> not ISpecification.
That was something I picked up from the work I merged from ajmitch. goal
is a bit more work as it is not a simple attribute, so I didn't bother
working on that, I just left goalstatus exported for now.
> 2. "I added two new methods to IHasSpecifications, as if we exposed
> the existing attributes we would serve the collections when just
> getting the specification." Can you explain? There are many places
> where we publish a collection associated with an entry. (Just within
> IPerson we have 'languages', 'gpg_keys', 'pending_gpg_keys',
> 'wiki_names', 'irc_nicknames', etc.) Just getting the person doesn't
> give you these collections--it gives you links to the collections.
Ok, I still think methods are more appropriate for this given parallels
with e.g. searchTasks, but I can change if you feel strongly.
Also, doesn't exposing collection attributes require them to have a
traversal? I don't think these do, as there is just /~person/+specs that
takes query arguments.
> 3. Rather than publishing updateLifecycle
> think you could publish it as the mutator for the 'completer' field. I
> know that you're not publishing 'completer' right now, but I imagine
> it will be published eventually, and publishing it now with a mutator
> means that end-users will have to learn one less strangely-named named
> operation.
>
> The only catch is that updateLifecycle
> lifecycle is not in a state for being completed. It would be better to
> return a 400 response code, which probably means raising a certain
> exception. (I'm fuzzy on the details but I can help you with this when
> the time comes.) We'd write a new method just for use in the web
> service, which wrapped an updateLifecycle
> appropriate exception-raising code.
>
> (At the very least, call this operation something like 'complete', and
> pass in the REQUEST_USER automatically--I don't think it makes much
> sense to complete a blueprint on someone else's behalf.)
I completely missed that this was exposed and there are no tests for
it. I would lean towards deferring it all to a later branch.
> = Questions about the tests =
>
> 1. You changed the default priority for a specification created through
> the factory to LOW, and then changed some tests that were assuming a
> factory-created specification started out UNDEFINED. Is it easy to
> leave the earlier behavior alone? Or are specifications with UNDEFINED
> priority considered invalid, or don't show up in lists, or anything else?
I can ...
| Leonard Richardson (leonardr) wrote : | # |
> > I don't have any opinion on the whiteboard thing, but I'll make sure someone
> with an opinion looks at this branch.
Curtis, do you have any objection to publishing the blueprint whiteboard through the web service?
> That was something I picked up from the work I merged from ajmitch. goal
> is a bit more work as it is not a simple attribute, so I didn't bother
> working on that, I just left goalstatus exported for now.
I'd rather remove goalstatus for now (to avoid cluttering up the interface), or do a little work to export goal as well. I think exporting goal is just a matter of changing the Attribute to an IReference(
If we can figure this out, and the schema is something that's already exported through the web service, then it should be easy to export goal.
> > 2. "I added two new methods to IHasSpecifications, as if we exposed
> > the existing attributes we would serve the collections when just
> > getting the specification." Can you explain? There are many places
> > where we publish a collection associated with an entry. (Just within
> > IPerson we have 'languages', 'gpg_keys', 'pending_gpg_keys',
> > 'wiki_names', 'irc_nicknames', etc.) Just getting the person doesn't
> > give you these collections--it gives you links to the collections.
>
> Ok, I still think methods are more appropriate for this given parallels
> with e.g. searchTasks, but I can change if you feel strongly.
searchTasks() needs to be a named operation because it takes arguments. These attributes here are more like IBug.bug_tasks.
> Also, doesn't exposing collection attributes require them to have a
> traversal? I don't think these do, as there is just /~person/+specs that
> takes query arguments.
Exposing a _top-level_ collection requires a traversal, but lazr.restful automatically takes care of traversal from an object to one of its associated collections.
> > 3. Rather than publishing updateLifecycle
> > think you could publish it as the mutator for the 'completer' field. I
> > know that you're not publishing 'completer' right now, but I imagine
> > it will be published eventually, and publishing it now with a mutator
> > means that end-users will have to learn one less strangely-named named
> > operation.
> >
> > The only catch is that updateLifecycle
> > lifecycle is not in a state for being completed. It would be better to
> > return a 400 response code, which probably means raising a certain
> > exception. (I'm fuzzy on the details but I can help you with this when
> > the time comes.) We'd write a new method just for use in the web
> > service, which wrapped an updateLifecycle
> > appropriate exception-raising code.
> >
> > (At the very least, call this operation something like 'complete', and
> > pass in the REQUEST_USER automatically--I don't think it makes much
> > sense to complete a blueprint on someone else's behalf.)
>
> I completely missed that this was exposed and there are no tests for
> it....
| James Westby (james-w) wrote : | # |
> I'd rather remove goalstatus for now (to avoid cluttering up the interface),
> or do a little work to export goal as well. I think exporting goal is just a
> matter of changing the Attribute to an IReference(
> know what that ??? is. Is it a distro series? Project series? Some abstract
> class representing the union of all possible series? Curtis, do you know?
It's a distroseries or productseries, or possibly other things too, I'm not sure.
I doubt there is a single schema for it.
> If we can figure this out, and the schema is something that's already exported
> through the web service, then it should be easy to export goal.
I'll remove goalstatus from this branch.
> Exposing a _top-level_ collection requires a traversal, but lazr.restful
> automatically takes care of traversal from an object to one of its associated
> collections.
Ok, great.
> Yes, I missed getSpecification when I wrote this.
>
> Try something like this:
>
> def getSpecOnWebser
> ...
> pillar = launchpadlib.
> pillar_name)
> return pillar.
Ok.
> Oh, another thing I forgot to mention is that once we/you upgrade Launchpad's
> launchpadlib, you should be able to pass relative URLs into Launchpad.load()
Yes, that's why I started down this path, but I can't land it as a prerequisite
branch yet.
> Fair enough. Would you be okay with a test that compared the entire
> representation as received by launchpadlib against some expected value? Take a
> look at lib/lp/
> display the full representation of an object derived from a JSON document.
What is the value in that test? That no attribute is exported without tests
being added?
> The difference between that test and your test is that your test does an end-
> to-end test with launchpadlib. From launchpadlib you can get an object that
> can be passed into pprint_entry with:
>
> entry._
>
> I know that's a little ugly but if you're interested in using this in tests,
> we can make it part of the official launchpadlib API.
Well, I'm happy making a direct webservice call and comparing the parsed
JSON, I'm just not sure of the value.
> I believe so. Call it WebServiceClien
> launchpadlib instance was started up in one of the existing layers, but we
> removed that because it slowed the tests down too much.
The problem with a layer is what credentials does it get? Are we going to
end up with a really expensive layer that creates 5 different launchpadlib
objects with different credentials, or 5 different layers?
> As a quick hack, defining it in the setup() method would at least let you
> share it within a test class.
No it wouldn't, setUp is called once per test, unless I misunderstand what
you mean.
Thanks,
James
| Curtis Hovey (sinzui) wrote : | # |
1. It is fine to export whiteboards because they are used. We vowed to stop adding them because they are a poor substitute for a description/wiki.
2. series goal is...well yuck. I do not have a lot of confidence that this implementation is safe. It must be impossible fo a user to directly change the productseries and distroseries--it is impossible for both to have a series. .proposeGoal() has to be exposed, and .goal is used to retrieve it. I think distroseries and productseries should not be exported. ISeries is the generic type for .goal, there are productseries and distroseries are the two implementers
2.5 likewise, I do not think product and distribution should be exported, target returns the correct value and retarget() allows you to set it. target is ISpecificationT
Blueprints uses the view to validate when the model or an interface invarant should be doing the validation. Since I do not see any view changes I think we need a branch to land prior to this one that makes the model safe to export.
* SpecificationRe
* updateLifecycle
| Robert Collins (lifeless) wrote : | # |
Setting to WIP to make it clear its been reviewed - please put back to needs-review when curtis' comments are addressed ;)
| Guilherme Salgado (salgado) wrote : | # |
https:/
I've also filed a bug for getting rid of the sole remaining call to updateLifecycle

Thanks a lot for this branch. This is a branch I was planning to create as a way of introducing Benji to the web service, but instead we went through your branch and reviewed it.
I don't have any opinion on the whiteboard thing, but I'll make sure someone with an opinion looks at this branch.
= Questions about the exported objects =
1. Obviously you don't need to export everything to make this branch goalstatus but goal.
useful, but I wonder why you exported ISpecification.
not ISpecification.
2. "I added two new methods to IHasSpecifications, as if we exposed
the existing attributes we would serve the collections when just
getting the specification." Can you explain? There are many places
where we publish a collection associated with an entry. (Just within
IPerson we have 'languages', 'gpg_keys', 'pending_gpg_keys',
'wiki_names', 'irc_nicknames', etc.) Just getting the person doesn't
give you these collections--it gives you links to the collections.
3. Rather than publishing updateLifecycle Status as a named operation, I
think you could publish it as the mutator for the 'completer' field. I
know that you're not publishing 'completer' right now, but I imagine
it will be published eventually, and publishing it now with a mutator
means that end-users will have to learn one less strangely-named named
operation.
The only catch is that updateLifecycle Status( ) returns None if the Status( ) call with
lifecycle is not in a state for being completed. It would be better to
return a 400 response code, which probably means raising a certain
exception. (I'm fuzzy on the details but I can help you with this when
the time comes.) We'd write a new method just for use in the web
service, which wrapped an updateLifecycle
appropriate exception-raising code.
(At the very least, call this operation something like 'complete', and
pass in the REQUEST_USER automatically--I don't think it makes much
sense to complete a blueprint on someone else's behalf.)
= Questions about the tests =
1. You changed the default priority for a specification created through
the factory to LOW, and then changed some tests that were assuming a
factory-created specification started out UNDEFINED. Is it easy to
leave the earlier behavior alone? Or are specifications with UNDEFINED
priority considered invalid, or don't show up in lists, or anything else?
2. You changed the factory's makeSpecification method to take a lot of et.new( ) to take the same arguments, which seems like
new arguments, which is fine, but you also changed
ISpecificationS
overkill. Can makeSpecification create a generic specification object
and then customize it?
3. Due to limitations of launchpadlib and the web service there's no
way around constructing the URL to a product when loading it in
launchpadlib. But, once you have that URL, is it possible to navigate
to a specific spec through launchpadlib object traversal? If so, I'd
like to see this in the test. If not, I think this branch should
publish some way of getting from an IHasSpecification to an
ISpecification, given the name--probably as a named operation.
4. In IHasSpecificati onsTests, you could save a lot of setup code by
writing a helper methods that ta...