Landscape Client

Merge lp://staging/~tribaal/landscape-client/fix-1228301-config-object-auto-list into lp://staging/~landscape/landscape-client/trunk

  1. fix-1228301-config-object-auto-list
  2. Merge into trunk
Proposed by Chris Glass
Status: Merged
Approved by: Chris Glass
Approved revision: 729
Merged at revision: 727
Proposed branch: lp://staging/~tribaal/landscape-client/fix-1228301-config-object-auto-list
Merge into: lp://staging/~landscape/landscape-client/trunk
Diff against target: 53 lines (+18/-3)
3 files modified
landscape/broker/tests/test_config.py (+13/-0)
landscape/deployment.py (+4/-2)
landscape/tests/test_configuration.py (+1/-1)
To merge this branch: bzr merge lp://staging/~tribaal/landscape-client/fix-1228301-config-object-auto-list
Reviewer Review Type Date Requested Status
Adam Collard (community) Approve
Free Ekanayaka (community) Approve
Review via email: mp+187022@code.staging.launchpad.net

Commit message

Prevent ConfigObj from being smart about list types

Description of the change

This branch fixes the linked bug.

We recently switched to using ConfigObj for configuration reading (it's much better), but it tries to be smart about lists: a config option like blah = foo,bar will ceate a configuration object with: config.blah = ['foo', 'bar'], while our code expects a "dumb" string, like config.blah = "foo,bar".

ConfigObj provides a way to cancel this behavior, but the switch was passed after __init__ was called, and was therefore ineffective.

To post a comment you must log in.
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Nice fix, +1!

[1]

+ config_obj = ConfigObj(config_source, list_values=False)

Please add a comment explaining why list_values is set to False.

review: Approve
Revision history for this message
Adam Collard (adam-collard) wrote :

Do we really want our code to do the parsing of comma separated values into a list? I was expecting to see some removed bits of our code and not disabling the good stuff in ConfigObj :) Did you look in to that solution?

review: Needs Information
Revision history for this message
Chris Glass (tribaal) wrote :

Well I would agree on principle, but the message we send the server contains the string version too - it sounded a bit silly to ",".join() the list.
Also, this is much, much cheaper.

But yeah, we could use an actual list instead, I suppose.

Revision history for this message
Adam Collard (adam-collard) wrote :

:( if we're sending it to the server as a string then this makes sense. Quite why we are sending it to the server as a string is another issue, so no point holding this up.

Looks good. +1

review: Approve
Revision history for this message
Chris Glass (tribaal) wrote :

Yeah, we could probably change that at a later point (tech debt?).

Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :
Download full text (204.5 KiB)

The attempt to merge lp:~tribaal/landscape-client/fix-1228301-config-object-auto-list into lp:landscape-client failed. Below is the output from the failed tests.

python setup.py build_ext -i
running build_ext
landscape.broker.tests.test_amp
  RemoteBrokerTest
    test_call_if_accepted ... [OK]
    test_call_if_accepted_with_not_accepted ... [OK]
    test_call_on_events ... [OK]
    test_exit ... [OK]
    test_fire_event ... [OK]
    test_get_accepted_message_types ... [OK]
    test_get_server_uuid ... [OK]
    test_is_message_pending ... [OK]
    test_listen_events ... [OK]
    test_method_call_error ... [OK]
    test_ping ... [OK]
    test_register ... [OK]
    test_register_client ... [OK]
    test_register_client_accepted_message_type ... [OK]
    test_reload_configuration ... [OK]
    test_send_message ... [OK]
    test_send_message_with_urgent ... [OK]
    test_stop_clients ... [OK]
  RemoteClientTest
    test_exit ... [OK]
    test_fire_event ... [OK]
    test_message ... [OK]
    test_method_call_error ... [OK]
    test_ping ... [OK]
landscape.broker.tests.test_client
  BrokerClientTest
    test_add ... [OK]
    test_dispatch_message ... [OK]
    test_dispatch_message_with_exception ... [OK]
    test_dispatch_message_with_no_handler ... [OK]
    test_exchange ... [OK]
    test_exchange_logs_errors_and_continues ... [OK]
    test_exchange_on_plugin_without_exchange_method ... [OK]
    test_exit ... [OK]
    test_fire_event ... [OK]
    test_fire_event_with_acceptance_changed ... [OK]
    test_fire_event_with_arguments ... [OK]
    test_fire_event_with_mixed_results ... [OK]
    test_get_named_plugin ... ...

lp://staging/~tribaal/landscape-client/fix-1228301-config-object-auto-list updated
729. By Chris Glass

Fixes tarmac failure - should have been caught before though.

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

to all changes: