Add runtime manager class #8

ssbarnea · 2021-06-25T15:21:38Z

Initial feedback should answer:

Is Runtime() class ok or you have a better proposal?
Does the README example look at simple a possible to use?
Runtime construction accepts two optional arguments
- project_dir, defaulting to cwd
- isolated bool, which determine if we should make use of our isolated cache for for installing roles or collections

While current minimal implementation only calls ansible from command line, it will be possible to use the same interface to run ansible execution environments in the future.

Adds a Runtime() class that can be used to interact with a specific version of Ansible and ease access to various commands.

webknjaz · 2021-07-02T14:00:17Z

src/ansible_compat/runtime.py

@@ -0,0 +1,89 @@
+"""Ansible runtime environment maanger."""


is not a mistake is on purpose! See https://en.wikipedia.org/wiki/Manger ;)

But the wiki page shows a word with just one "a" :)

webknjaz · 2021-07-07T09:53:43Z

README.md

+    # runtime.install_collection("community.general")
+
+    # Execute a command
+    result = runtime.exec(["ansible-doc", "--list"])


I wouldn't call the method exec() because it creates a misleading impression that it is related to the syscall that replaces the current process with a new one.

Suggest a name, I am aware of its bad naming. I considered Runtime.run() as it will likely be very similar to subprocess.run, but ...

I commented below but it should instead just follow check_call() style.

webknjaz · 2021-07-07T09:55:25Z

src/ansible_compat/errors.py

+        self.proc = proc
+
+
+class AnsibleCommandError(RuntimeError):


It'd be useful to have a common lib-defined base class for all the exceptions.

Additionally, this should also inherit subprocess.SubprocessError

Actually, I'm not even sure if it should depend on RuntimeError because its purpose is to indicate unspecific unexpected things happening at runtime.

Clearly not SubprocessError because we may endup executing code with something else (containers) but I am open to suggestions.

When you execute things in containers, you still run subprocesses. It's a good abstraction.

webknjaz · 2021-07-07T10:15:23Z

src/ansible_compat/errors.py

+    def __init__(
+        self, message: Optional[str] = None, proc: Optional[Any] = None
+    ) -> None:
+        """."""


Boilerplate to pass linting, is means "TODO: add meaningful description here." ;)

Yeah, but why not fill it in right away?

webknjaz · 2021-07-07T10:46:57Z

src/ansible_compat/runtime.py

+        self.config = AnsibleConfig()
+
+    # pylint: disable=no-self-use
+    def exec(self, args: Union[str, List[str]]) -> CompletedProcess:


I'd rather have a check_call() style thingy. Plus we need to address the naming concern I raised above.

webknjaz · 2021-07-07T10:48:04Z

src/ansible_compat/runtime.py

+            return self._version
+
+        proc = self.exec(["ansible", "--version"])
+        if proc.returncode == 0:


Having if .. != 0: raise could simplify this block.

webknjaz · 2021-07-07T10:51:24Z

test/test_runtime.py

+    assert version == runtime.version
+
+
+def test_runtime_version_fail(mocker: MockerFixture) -> None:


Nit: I normally advocate for using pytest's builtin monkeypatch for as long as you don't use things uniquely accessible only via unittest.mock.patch.

webknjaz · 2021-07-07T10:52:07Z

test/test_runtime.py

+    )
+    runtime = Runtime()
+    with pytest.raises(RuntimeError) as exc:
+        _ = runtime.version


_ = part isn't actually needed here — you could just test accessing the property and that would work.

_ was added to keep the linter happy, lost of ugly things goes towards passing our linters.

I'd prefer noqa/disable comments because they communicate the intent more clearly.

webknjaz · 2021-07-07T10:53:23Z

test/test_runtime.py

+        autospec=True,
+    )
+    runtime = Runtime()
+    with pytest.raises(RuntimeError) as exc:


Looks like this should've been MissingAnsibleError and the idiom is to always use match= arg here.

webknjaz · 2021-07-07T13:10:58Z

src/ansible_compat/runtime.py

+    CompletedProcess = subprocess.CompletedProcess
+
+
+class Runtime:


I think this may need to implement a CM at some point.

ssbarnea added the enhancement New feature or request label Jun 25, 2021

ssbarnea force-pushed the fix/api branch 3 times, most recently from 7173691 to 9f023fb Compare June 30, 2021 08:06

ssbarnea changed the title ~~WIP: Propose API~~ Add runtime manager class Jun 30, 2021

ssbarnea marked this pull request as ready for review June 30, 2021 08:08

ssbarnea requested a review from webknjaz as a code owner June 30, 2021 08:08

ssbarnea force-pushed the fix/api branch 3 times, most recently from 5337f24 to 8ae2233 Compare June 30, 2021 15:05

ssbarnea requested review from tadeboro and felixfontein June 30, 2021 15:08

ssbarnea force-pushed the fix/api branch 6 times, most recently from 5c065f7 to ade04fd Compare July 1, 2021 08:48

Add runtime manager class

2f490f4

Adds a Runtime() class that can be used to interact with a specific version of Ansible and ease access to various commands.

ssbarnea force-pushed the fix/api branch from ade04fd to 2f490f4 Compare July 1, 2021 08:55

ssbarnea merged commit f8d030e into main Jul 1, 2021

ssbarnea deleted the fix/api branch July 1, 2021 09:00

webknjaz reviewed Jul 2, 2021

View reviewed changes

webknjaz reviewed Jul 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add runtime manager class #8

Add runtime manager class #8

ssbarnea commented Jun 25, 2021 •

edited

Loading

webknjaz Jul 2, 2021

ssbarnea Jul 2, 2021

webknjaz Jul 3, 2021

webknjaz Jul 7, 2021

ssbarnea Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

ssbarnea Jul 7, 2021

webknjaz Jul 7, 2021 •

edited

Loading

webknjaz Jul 7, 2021

ssbarnea Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

ssbarnea Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

webknjaz Jul 7, 2021

		assert version == runtime.version


		def test_runtime_version_fail(mocker: MockerFixture) -> None:

		CompletedProcess = subprocess.CompletedProcess


		class Runtime:

Add runtime manager class #8

Add runtime manager class #8

Conversation

ssbarnea commented Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssbarnea commented Jun 25, 2021 •

edited

Loading

webknjaz Jul 7, 2021 •

edited

Loading