Merge lp://staging/~vbocek/ubuntu-system-image/support_http into lp://staging/~registry/ubuntu-system-image/client

Proposed by Vojtech Bocek
Status: Merged
Approved by: Barry Warsaw
Approved revision: 242
Merged at revision: 241
Proposed branch: lp://staging/~vbocek/ubuntu-system-image/support_http
Merge into: lp://staging/~registry/ubuntu-system-image/client
Diff against target: 290 lines (+180/-11)
7 files modified
ini-manpage.rst (+4/-2)
systemimage/bag.py (+8/-0)
systemimage/config.py (+30/-9)
systemimage/tests/data/config_05.ini (+36/-0)
systemimage/tests/data/config_06.ini (+36/-0)
systemimage/tests/data/config_07.ini (+36/-0)
systemimage/tests/test_config.py (+30/-0)
To merge this branch: bzr merge lp://staging/~vbocek/ubuntu-system-image/support_http
Reviewer Review Type Date Requested Status
Barry Warsaw (community) Approve
Review via email: mp+208242@code.staging.launchpad.net

Description of the change

* Add support for "disabled" value in http_port and https_port config options, which disables that protocol.
* Support overriding of [system] section from channel.ini

- The tests are incomplete.

To post a comment you must log in.
Revision history for this message
Barry Warsaw (barry) wrote :

Just a quick comment as I'm going through your branch.

=== modified file 'systemimage/config.py'
--- systemimage/config.py 2014-02-13 18:16:17 +0000
+++ systemimage/config.py 2014-02-25 22:24:24 +0000
> @@ -64,32 +78,39 @@
> if files_read != [path]:
> raise FileNotFoundError(path)
> self.config_file = path
> - self.service = Bag(converters=dict(http_port=int,
> - https_port=int,
> + self.service.update(converters=dict(http_port=port_value_converter,
> + https_port=port_value_converter,
> build_number=int),
> **parser['service'])
> + if (self.service.http_port is DISABLED_PROTOCOL and
> + self.service.https_port is DISABLED_PROTOCOL):
> + raise ValueError('both http and https ports were disabled')
> # Construct the HTTP and HTTPS base urls, which most applications will
> # actually use.
> if self.service.http_port == 80:
> self.service['http_base'] = 'http://{}'.format(self.service.base)
> - else:
> + elif self.service.http_port is not DISABLED_PROTOCOL:
> self.service['http_base'] = 'http://{}:{}'.format(
> self.service.base, self.service.http_port)
> if self.service.https_port == 443:
> self.service['https_base'] = 'https://{}'.format(self.service.base)
> - else:
> + elif self.service.http_port is not DISABLED_PROTOCOL:
> self.service['https_base'] = 'https://{}:{}'.format(
> self.service.base, self.service.https_port)
> - # Short-circuit, since we're loading a channel.ini file.
> - self._override = override
> - if override:
> - return
> - self.system = Bag(converters=dict(timeout=as_timedelta,
> + if self.service.http_port is DISABLED_PROTOCOL:
> + self.service['http_base'] = self.service['https_base']
> + elif self.service.https_port is DISABLED_PROTOCOL:
> + self.service['https_base'] = self.service['http_base']
> + self.system.update(converters=dict(timeout=as_timedelta,
> build_file=expand_path,
> loglevel=as_loglevel,
> settings_db=expand_path,
> tempdir=expand_path),
> **parser['system'])

The changes above are problematic because currently, Bags have the semantic
that their values cannot change after the initial setting. So updating as is
done here raises a ValueError. It's worse too because the test infrastructure
requires a Configuration object before any tests run (for unfortunate reasons
having to do with the D-Bus tests).

I'm not exactly sure of the best way to resolve this. I should probably just
remove this read-only restriction, since it's not really serving a useful
purpose.

Revision history for this message
Barry Warsaw (barry) wrote :

On Feb 26, 2014, at 12:03 AM, Barry Warsaw wrote:

>I'm not exactly sure of the best way to resolve this. I should probably just
>remove this read-only restriction, since it's not really serving a useful
>purpose.

nm, I know how to resolve this.

Revision history for this message
Vojtech Bocek (vbocek) wrote :

It didn't raise any error while I was testing it, the exception in __setitem__ affects only external changes via [] operator.

>>> from bag import Bag
>>> b=Bag(a=4, b=8)
>>> b.update(a=0, b=2)
>>> b['a']
0
>>> b['b']
2

Revision history for this message
Barry Warsaw (barry) wrote :

Thanks very much for this branch. I'm going to merge it to trunk after some
tweaks, and fleshing out of the test suite.

I ended up making these changes to your branch:

  * Renamed DISABLED_PROTOCOL to DISABLED
  * Some minor code style issues.
  * Allow for the [system] section to be missing from a channel.ini file.
  * Bag class API change: Setting an attribute via the setitem API no longer
    makes that item read-only. IOW, only keywords set in the constructor are
    read-only.
  * Flesh out tests.
  * Tweak the manpage.

Once I've merged this to trunk, you can take a look at the specific diff
there. Please let me know if I've missed anything or if you have any
questions or comments.

Revision history for this message
Barry Warsaw (barry) wrote :

Approved, with tweaks. See lp:~barry/ubuntu-system-image/lp1278589

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches