pass arguments as JSON array to process probe

[twuyts]

This PR allows for more freedom in passing arguments to a process probe.
For example: when running a docker container, it is possible to pass in environment variables through
the -e cmdline option. If you need to add more than one option, you just add multiple -e arguments. I have a few simple docker containers that implement a probe, but configuring multiple -e arguments in the experiment JSON to the container is not possible in a JSON object.

By also allowing the arguments object in the probe configuration to be an array, you can pass any argument you like, without being confined to the option:value structure. See ProcEchoArrayProbe in tests/probes.py for an example.

[Lawouach]

Awesome idea and work @twuyts!

While I review it, would you be kind enough to sign your commit so the DCO check is happy? :)

[twuyts]

DCO and whitespace stuff is fixed, but I seem to have a failed test in Python 3.5. I've only tested in 3.6. I'll have a quick look.

[twuyts]

It's a problem with the way Python handles dicts. Apparently it has changed from 3.5 -> 3.6
see this post on StackOverflow

The official description of the change is mentioned here

This means that my test that checks if the arguments were passed correctly will fail most of the times when using a JSON object (vs array). Passing them as an array will pass the arguments in the given order, though.

I will remove the failing test, as I don't see a way to do this correctly.

[Lawouach]

Dict are ordered (insertion that is) ordered now indeed. Could we use an OrderedDict when using python 3.5-?

[twuyts]

I assume we could.

def load_experiment(path: str) -> Experiment:
    """
    Parse the given experiment from `path` and return it.
    """
    with io.open(path) as f:
        if (sys.version_info >= (3,6)):
            return json.load(f)
        else:
            return json.load(f, object_pairs_hook=OrderedDict)

That would make an OrderedDict of the complete experiment object.
I'm not too confident about this: there doesn't seem to be a single test that actually loads an experiment from a file, so I'm not sure if there aren't any unintended side-effects.

What do you propose?

[Lawouach]

That's actually an interesting idea, I didn't know the json interface allowed for this kind of possibility. I will toy around (and indeed write a test would be a good idea).

How urgent is this for you to be merged? I might need until Friday to get that done, is that okay?

[twuyts]

That's fine, thanks!

[Lawouach]

I have worked on this PR yesterday but couldn't find the right way to make that test pass either yet. I think it should be the case so I'll continue on that next week :)

[Lawouach]

While trying to find a way to make it work, I realised we shouldn't actually assume order at all when using a dict-based arguments sequence.

Basically, we should document that if the ordering of arguments is mandatory, then the list approach must be used. When ordering is meaningless, either ones should be used (in fact, we could even deprecate the dict approach).

what do you think?

[twuyts]

Good idea. I'm currently only using the list approach anyway, I don't see any advantages in the dict approach, except perhaps a slight improvement in readability. I already format my arguments to be as readable as possible, e.g.: ''' { "type": "probe", "name": "Nibbles", "provider":{ "type":"process", "path":"docker", "arguments":[ "run", "--env-file", "${nibbles_env}", "-e", "NIBBLES_PROBE_RUN_ONCE=true", "${nibbles_image}" ], "timeout":400 } ''' so you might as well deprecate the dict approach. 2018-02-18 22:01 GMT+01:00 Sylvain Hellegouarch <notifications@github.com>:

…

While trying to find a way to make it work, I realised we shouldn't actually assume order at all when using a dict-based arguments sequence. Basically, we should document that if the ordering of arguments is mandatory, then the list approach must be used. When ordering is meaningless, either ones should be used (in fact, we could even deprecate the dict approach). what do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADEzcAE_CWPmIiq3PO0B_akeOgqw_2f5ks5tWI-1gaJpZM4SFVbb> .

[Lawouach]

Sounds appropriate yeah. I think I can merge your PR as-is and I will add the deprecation warning for the dict approach afterwards.