simplestreams

Merge lp://qastaging/~danilo/simplestreams/custom-user-agent into lp://qastaging/simplestreams

custom-user-agent
Merge into trunk

Proposed by Данило Шеган on 2016-04-20

Status:	Merged
Merged at revision:	429
Proposed branch:	lp://qastaging/~danilo/simplestreams/custom-user-agent
Merge into:	lp://qastaging/simplestreams
Diff against target:	383 lines (+224/-21) 6 files modified simplestreams/contentsource.py (+11/-5) simplestreams/mirrors/__init__.py (+23/-5) simplestreams/mirrors/glance.py (+1/-1) tests/httpserver.py (+43/-0) tests/unittests/test_contentsource.py (+30/-10) tests/unittests/test_mirrorreaders.py (+116/-0)
To merge this branch:	bzr merge lp://qastaging/~danilo/simplestreams/custom-user-agent
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
simplestreams-dev		2016-04-20	Pending
Review via email: mp+292403@code.qastaging.launchpad.net

Description of the change

= Add user-agent support to simplestreams =

This branch adds user-agent support as required by Landscape's use of glance-simplestreams-sync charm to python simplestreams: we want to have a custom user agent when we hit cloud-images.u.c to be able to tell which clouds are managed by Landscape.

Because of the code structure, there's some not-very-nice code in here, mainly the use of url_reader_factory function, but without knowing what parts of simplestreams API we want to keep backward compatible, I refrained from changing anything of the existing code too much.

There are tests for all the new code, and for some of the old code that was untested (if I missed something, do let me know).

I've also manually tested this with our charm from lp:~danilo/charms/trusty/glance-simplestreams-sync/use-upstream-user-agent-support (cs:~danilo/trusty/glance-simplestreams-sync-18
with manually copied and installed python-simplestreams debs) inside Autopilot to make sure user-agent indeed continues to work (our existing charm monkey-patches simplestreams internal structures, which is far from perfect).

Revision history for this message

Scott Moser (smoser) wrote on 2016-04-21:

Hi,
Would anything be easier if we just always set a user-agent, and allowed you to override it?
Ie, we'd just define USER_AGENT somewhere and use that as the default everywhere, and always pass USER_AGENT.

Revision history for this message

Данило Шеган (danilo) wrote on 2016-04-21:

It would sure be easier, but it would still be more of a monkey-patching approach (i.e. modifying a value on the library code is kind-of bad).

For it to be cleaner, imho at least, we'd need an entirely different high level relationship between UrlReaders, MirrorReaders and ContentSources, which I think would be a significant refactoring, and highly likely to break some API backwards compatibility.

Thanks for the Python3 and all the other fixes: I'll take a look and then merge into my branch. Will ping tomorrow (close to EOD now) when I've looked over it.

lp://qastaging/~danilo/simplestreams/custom-user-agent updated on 2016-04-22

430. By Данило Шеган on 2016-04-22: Merge python3 fixes from Scott Moser.

Revision history for this message

Данило Шеган (danilo) wrote on 2016-04-22:

I've merged your changes in, and will now be doing another round of testing (the loop is a bit long for all the things).

Revision history for this message

Данило Шеган (danilo) wrote on 2016-04-22:

Btw, one thing that just occurred to me: it might be useful to have a way for callsites to check if user-agent is supported (if they want to continue to work with old and new versions). Any existing framework in python-simplestreams for that?

Revision history for this message

Scott Moser (smoser) wrote on 2016-04-22:

Данило,
There is not any way to check that.
i'm not opposed to adding a "version" of some sort that would increment and the user could check (and use hasattr to see if its there at all).

Revision history for this message

Данило Шеган (danilo) wrote on 2016-04-26:

FWIW, in my tests with this branch, I've now hit https://pastebin.canonical.com/155206/

It's more likely this has something to do with me using trunk to base my branch on instead of the trusty one. Looking into it.

lp://qastaging/~danilo/simplestreams/custom-user-agent updated on 2016-04-26

431. By Данило Шеган on 2016-04-26: Fix config[hypervisor_mapping] unconditional check.

Revision history for this message

Данило Шеган (danilo) wrote on 2016-04-26:

I've fixed this with https://pastebin.canonical.com/155210/

Revision history for this message

Данило Шеган (danilo) wrote on 2016-04-26:

If you feel this is not the right fix and this option should always be set in the config for GlanceMirror, I'll modify the glance-simplestreams-sync charm to pass it on.

Revision history for this message

Scott Moser (smoser) wrote on 2016-04-28:

I think glance-simplestreams-sync should pass it in, but the guard against possibly existing data is fine.
See the bzr history for where that came from. I think you want to have it to make multi-hypervisor clouds possible.

Revision history for this message

Данило Шеган (danilo) wrote on 2016-05-04:

Heya, sorry for taking a while to respond (long Easter weekend for Serbia :).

We'll make glance-simplestreams-sync pass it in in our work to support multiple hypervisors. That work is ongoing (independently from this), so I won't do that to avoid having to debug that as well.

As for my above fix, I'll change it to become .get(..., False) to default to False instead of None, even though it behaves the same in the boolean context, I prefer explicit boolean values instead.

lp://qastaging/~danilo/simplestreams/custom-user-agent updated on 2016-05-04

432. By Данило Шеган on 2016-05-04: Default to False for hypervisor_mapping.
433. By Данило Шеган on 2016-05-04: Default to "sstreams-0.1" user-agent string.
434. By Данило Шеган on 2016-05-04: Use a bit more verbose default user-agent string.
435. By Данило Шеган on 2016-05-04: Set the user agent string as a constant on the module level.

Revision history for this message

Данило Шеган (danilo) wrote on 2016-05-05:

Heya Scott, you seem to have missed to merge all the changes (namely, any changes after your proposed fixes, including the hypervisor_mapping fix and changing the default user-agent for python-simplestreams).

Revision history for this message

Scott Moser (smoser) wrote on 2016-05-05:

Ugh. Will fix

On May 5, 2016 3:30:30 AM CDT, "Данило Шеган" <email address hidden> wrote:
>Heya Scott, you seem to have missed to merge all the changes (namely,
>any changes after your proposed fixes, including the hypervisor_mapping
>fix and changing the default user-agent for python-simplestreams).
>--
>https://code.launchpad.net/~danilo/simplestreams/custom-user-agent/+merge/292403
>Your team simplestreams-dev is requested to review the proposed merge
>of lp:~danilo/simplestreams/custom-user-agent into lp:simplestreams.