feat(spans): Add role level spans, item spans, rework test #22
Conversation
refactor(plugin): apply pep8 recommendations fix(test): correct module name fix(test): c: dont use community modules
|
|
||
| return (hosts, duration_events, duration_stacks) | ||
| return (hosts, duration_events) | ||
|
|
||
|
|
||
| def add_duration_event( |
There was a problem hiding this comment.
only this sub really changed
| @@ -243,7 +244,7 @@ def _start_role_span(self, host, role): | |||
| "id": abs(hash(role._uuid)), | |||
| "args": { | |||
| "role": name, | |||
| "path": role.get_path(), | |||
| "path": role._role_path, | |||
There was a problem hiding this comment.
I'm not much of a python programmer: how legit/good/frowned-upon is it to use underscore variables? They're nominally-private, right? I suppose this could break in future versions? But it's the only way to get this data on old Ansible?
I wonder if we could check if get_path function exists and use it, falling back to this otherwise? WDYT
There was a problem hiding this comment.
Yes, these are private fields. Function get_path for role does not exists under 2.12 core. So yea this could break in futur version.
I will try to remove usage of private var when possible.
| @@ -12,7 +12,7 @@ | |||
| @pytest.mark.ansible_strategy('free') | |||
| def test_basic_multiple_free(ansible_play): | |||
| trace_hosts: Dict[int, HostEvent] | |||
| trace_events: Dict[int, Any] | |||
| trace_events: Dict[int, List[List[Any]]] | |||
There was a problem hiding this comment.
I think this is getting complex enough (and repeated enough) that it might be worth declaring a named type for these, WDYT?
There was a problem hiding this comment.
Yea, I agree
| # Write end event | ||
| self._write_event({ | ||
| "name": self._current_play.get_name().strip(), | ||
| "cat": "play", | ||
| "id": self._play_id, | ||
| "id": abs(hash(self._current_play._uuid)), |
There was a problem hiding this comment.
I'll admit I copy/pasted this one to stay coherent. But I guess it was for shorter iand more distinct id
| self._end_play_span() | ||
| self._f.write("\n]") | ||
| self._f.close() | ||
|
|
||
|
|
||
| @dataclass | ||
| @ dataclass |
There was a problem hiding this comment.
I think it's autopep8 lint. I do not know pep8 by heart, so I will double check
| if task._role is not None: | ||
|
|
||
| # Still in current role, nothing to do | ||
| if(self._hosts[host._uuid].role_stack and |
There was a problem hiding this comment.
I'm not a python style expert, but isn't it normal to have a space after the if ?
There was a problem hiding this comment.
Could be the autopep8 or just me when wrapping, if its not lint, I can change that.
| @@ -12,14 +12,15 @@ | |||
| from __future__ import (absolute_import, division, print_function) | |||
| __metaclass__ = type | |||
There was a problem hiding this comment.
General comment: There's a lot of changes in this commit; would it be possible to split out the reformatting from the actual new changes? If not, no worries, it'll just take me a bit longer to review.
There was a problem hiding this comment.
Yea, I know it was too much when I read the changes, I'll split it
👋 !
Close #18
Close #1
Close #23
Rework tests to take in account there can be multiple duration events with the same uuid
As we are lacking of callback this is mostly hack atm
Here some examples:
And here an use case:
Sorry for the noise, with all tests will fix it asap,
Waiting for your review !