labgrid icon indicating copy to clipboard operation
labgrid copied to clipboard

factory: pass copy of driver/resource args to avoid modifying config in place

Open Bastian-Krause opened this issue 2 years ago • 1 comments

Description normalize_config() and make_target() modify the referenced dicts returned by _convert_to_named_list(), meaning they modify the config in case resources or drivers are dictionaries, not lists:

resources: # or drivers
  FooPort: {}
  BarPort:
    name: "bar"

For the list case, copies of the relevant items are already used.

The modification happens when popping name, cls and bindings from the arguments.

This can be observed with an env.yaml containing a RemotePlace resource having a name argument:

targets:
  main:
    resources:
      RemotePlace:
        name: myplace

Now we pass this environment config to labgrid-client:

  $ labgrid-client --debug --config env.yaml console
  Selected role main and place myplace from configuration file
    DEBUG: expanded remote resources for place None: [NetworkPowerPort(target=Target(name='main', env=Environment(config_file='env.yaml')), name='NetworkPowerPort', state=<BindingState.bound: 1>, avail=True, model='netio', host='example.com', index='3')]

Note the "place myplace" in the first message and the "place None" in the second message. Between these messages, the RemotePlace's name was dropped and replaced with a default (None).

Note that RemotePlace is only a simple example for a resource or driver with an argument that is modified in the config.

To avoid config modification, make _convert_to_named_list() return a list of copied argument dictionaries that can be normalized and turned into a target without modifying the config.

Add check to factory tests that make_target() does not modify given config. Also add a test for target_factory.normalize_config().

Checklist

  • [x] Tests for the feature
  • [x] PR has been tested

Bastian-Krause avatar Sep 13 '22 11:09 Bastian-Krause

Codecov Report

Merging #989 (f5b63ef) into master (1d78c76) will increase coverage by 0.1%. The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master    #989     +/-   ##
========================================
+ Coverage    62.2%   62.3%   +0.1%     
========================================
  Files         151     151             
  Lines       11273   11274      +1     
========================================
+ Hits         7018    7033     +15     
+ Misses       4255    4241     -14     
Impacted Files Coverage Δ
labgrid/factory.py 98.3% <100.0%> (+11.9%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Sep 13 '22 11:09 codecov[bot]