Code review comment for ~jocave/plainbox-provider-checkbox:move-wireless-jobs-to-newer-templated-version

Revision history for this message
Jonathan Cave (jocave) wrote :

1. Previous job definitions were running commands as root, are we able to run the python wrapper as the normal user now?

The python wrapper is inherited from p-p-s where is was run non-root for quite a while. I believe it should be find running as non-root. I tried running this on my desktop as my normal user and it worked fine (apart from a bug in zesty's nmcli itself).

2. Lots of new jobs are using the environ filed which is not needed when running command as a non root user (See http://plainbox.readthedocs.io/en/latest/manpages/plainbox-job-units.html). You can delete those lines.

Thanks, updated branch to remove those. (squashed into first commit)

3. What happens if $NET_DRIVER_INFO is not set? should we condition the call to bin/net_driver_info only if the variable exists?

The environment variable is used to request the script print the information it can find about drivers that are of special interest to the user. If the variable is missing the script should still work and only print information for drivers that sysfs indicates are of class network.

4. Can all _manual connection tests benefit from the python tool? to get rid again of the inline bash commands?

I decide to not touch these jobs as I didn't know anything about what their target audience is. They are quite different to any of the other tests as the manual part of the test seems to refer to configuration Access Points.

« Back to merge proposal