Fixing stdout lookup for Juju 2.7

[ed1000]

It was failling because when the code is 1, it didn't returned
any Stdout, therefore it fails because of the missing key.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@dd10128). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #301   +/-   ##
=========================================
  Coverage          ?   85.21%           
=========================================
  Files             ?       26           
  Lines             ?     1650           
  Branches          ?        0           
=========================================
  Hits              ?     1406           
  Misses            ?      244           
  Partials          ?        0

Impacted Files	Coverage Δ
zaza/model.py	90.61% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd10128...b607cf7. Read the comment docs.

[ajkavanagh]

LGTM

[thedac]

There are multiple instances of ['Stdout'] calls. We need to be more comprehensive.

And the core issue is this is no longer true:
:returns: action.data['results'] {'Code': '', 'Stderr': '', 'Stdout': ''}

It might make sense to have a helper function which handles 2.6 and 2.7 action.data.

[ed1000]

It could be written to take into account 'Code' and 'Stderr' but it doesn't seem a wrong approach. The change that I detected is that when you do a pgrep or pidof for a service that is stopped, Juju 2.7 doesn't return any 'Stdout' or 'Stderr' because the pgrep or pidof don't return anything, they just exit with error code 1.

The change that I'm proposing just handles errors better, by having a default when the key doesn't exist.

[ajkavanagh]

There are multiple instances of ['Stdout'] calls. We need to be more comprehensive.

I'd agree with this.

And the core issue is this is no longer true:
:returns: action.data['results'] {'Code': '', 'Stderr': '', 'Stdout': ''}

Yes, it requires a more defensive strategy for all of the results.

It might make sense to have a helper function which handles 2.6 and 2.7 action.data.

Based on this, I'd say that the async_run_in_unit should be code from:

    async with run_in_model(model_name) as model:
        unit = get_unit_from_name(unit_name, model)
        action = await unit.run(command, timeout=timeout)
        if action.data.get('results'):
            return action.data.get('results')
        else:
            return {}

to:

    if action.data.get('results'):
        res = action.data['results']
        return {'Code': res.get('Code', ''), 'Stdout': ...}
    else:
        return {}

BUT, I'm really suspicious of the return {} as that also might break code, but there could already be code that depends on the empty dictionary?

[gnuoy]

Thanks for the fix

[gnuoy]

Actually, I just realised this has been superseded, a fix has landed to async_run_on_unit for this issue

[pengale]

This makes sense to me.

[ajkavanagh]

Added stale label as nothing has happened in over 2 years; PR may be closed if no further work is needed.