describe and implement the external protocol #94

supakeen · 2024-05-28T08:44:32Z

Describes an initial external protocol.

mvo5

Thank you for documenting this! Some inline questions/suggestions for your consideration. Hope they are useful.

doc/03-omnifest/02-external.md

mvo5 · 2024-05-28T08:56:36Z

doc/03-omnifest/02-external.md

+Directives are small executables that receive JSON and are expected to output
+JSON again in a specific format. `otk` will look for these directives in
+`/usr/libexec/otk`. Externals are explicitly mapped to executables in the
+registry that is defined in `src/otk/external.py`.


I'm not sure that this description of the "mapping" should be here. I think it's an implementation detail and I personally wold love to discuss if we cannot just auto-discover them. osbuild for example does not have a registry of stages and I'm not sure what it gives us that we have a registry. More control for sure but also less options for external people to develop their own externals. If we want to keep the registry in code maybe something like: Externals need official registration the otk source code to work or something (i.e. maybe more emphasis on the intend).

I've removed this section and we can do lookups as described in the new Paths section.

mvo5 · 2024-05-28T08:59:57Z

doc/03-omnifest/02-external.md

+## Protocol
+
+Directives are small executables that receive JSON and are expected to output
+JSON again in a specific format. `otk` will look for these directives in


I'm not sure that the path should be under "protocol", maybe worth adding a ## Files section or something? And it might be nice to describe how to run them from a git checkout or via an env var. Maybe OTK_EXTERNAL_PATH?

I've added a Paths section that includes this information.

mvo5 · 2024-05-28T09:00:25Z

doc/03-omnifest/02-external.md

+
+Directives are small executables that receive JSON and are expected to output
+JSON again in a specific format. `otk` will look for these directives in
+`/usr/libexec/otk`. Externals are explicitly mapped to executables in the


Do all distros nowdays have /usr/libexec (honest question, idk)

That's a good question. I don't know, what can we call it that would make sense across distributions?

Dove into this a bit; from a quick test it seems Debian, Ubuntu, RHEL (UBI), Fedora, CentOS, Gentoo all have /usr/libexec. Alpine and Arch do not.

We can either do our own search path (/usr/libexec and then /usr/lib, perhaps with /usr/local-variants for each) or let eventual packages for Alpine or Arch add a patch.

OK, I've updated the docs to search all relevant paths starting with an environment-defined one.

mvo5 · 2024-05-28T09:02:23Z

doc/03-omnifest/02-external.md

+{
+  "tree": {
+    "otk.external.name": {
+      "child": [],


I'm not sure I understand if "child" and "options" are examples or have a special meaning. Maybe above the json a yaml fragment could be added that has the yaml source from the otk file and then here we see the json that is generated from that yaml. Does that make sense?

They are indeed examples without special meaning. I'll add a YAML blurb!

YAML blurb has been added.

doc/03-omnifest/02-external.md

mvo5 · 2024-05-28T09:06:07Z

doc/03-omnifest/02-external.md

+}
+```
+
+*If the external returns `{}`, an empty object, `otk` will assume that there is


Does that mean {} or {"tree": {}} ? I assume the former, what will happen if otk gets the later?

It means the former. If it gets the latter then it will put a {} (empty object) into the output, I'll add an example of that.

I've added YAML examples for this.

achilleas-k · 2024-05-28T10:39:56Z

doc/03-omnifest/02-external.md

+When we have an [omnifest](./index.md) that looks like this:
+
+```yaml
+otk.external.name:
+  child:
+    - 1
+  options: "are here"
+```
+
+The external gets called with the following JSON:
+
+```json
+{
+  "tree": {
+    "otk.external.name": {
+      "child": [1],
+      "options": "are here"
+    }
+  }
+}
+```


Bit of a rephrase to avoid "you", "we", etc:

Given the following omnifest:

otk.external.custom-command: child: - 1 options: "are here"

The external with name custom-command gets called with the following JSON as input:

{ "tree": { "otk.external.custom-command": { "child": [1], "options": "are here" } } }

The reference to the name custom-command isn't the point here but I think repeating rules in various sections helps reinforce them. In other words, it's a reminder that the <something> part of otk.external.<something> is both significant and arbitrary (as far as the internals are concerned) and only needs to match the name of an external executable.

Super nitpick - the initial part of the suggestion to change When we have an [omnifest](./index.md) that looks like this: -> Given the following [omnifest](https://github.com/osbuild/otk/pull/94/index.md): seems to be gotten lost (which is not a big deal but I wanted to mention it :)

doc/03-omnifest/02-external.md

mvo5

Thank you! I like it, still some ideas/questions/suggestions inline, hope they are useful :)

mvo5 · 2024-05-28T10:55:06Z

doc/03-omnifest/02-external.md

+}
+```
+
+And is expected to return the following JSON:


Maybe And is expected to return a JSON dict with a single top-level "tree" object that can contain arbitrary values. Additional top-level keys are not allowed. Example:

Hm, this says "Done" but GH shows me the original version, is that intentional?

Some commit seems to have gone missing, really done now :)

doc/03-omnifest/02-external.md

mvo5 · 2024-05-28T10:59:42Z

doc/03-omnifest/02-external.md

+}
+```
+
+Which will lead to the following YAML structure:


Maybe: Which will replace the previous otk.external.name subtree with the output of new subtree from the external comand. Example:

Similar to above, this says "Done" but AFAICT the text is unchanged, is it "done" in an indirect way?

Some commit seems to have gone missing, really done now :)

doc/03-omnifest/02-external.md

mvo5 · 2024-05-28T11:05:45Z

doc/03-omnifest/02-external.md

+- `/usr/lib/otk`
+
+The filename for an external executable is based on the external name. When the
+following directive is encountered: `otk.external.<name>.<argument>` then


Do you have an example for <argument> ? I'm not sure I follow why we need this mapping. As a stawman, maybe something very simple. The search path is the same as above but contains "/external/" as the last component (so /usr/libexec/otk/external" and then just `osbuild_dnf4" ? Or are we limiting ourselfs here is do something very simple like this?

I think in the interest of keeping things simple for the initial implementation, we can avoid adding features that can be expressed easily with other features, especially if the alternative isn't too complicated. A subcommand here can easily be expressed using an option for the external.

... otk.external.myscript.subcommand_1: option: "value"

is not that much cleaner than:

... otk.external.myscript: command: "subcommand_1" option: "value"

I think the fact that the first form requires an extra paragraph in the specification and docs is a sign that it's not necessary given how the alternative isn't really that complicated.

I don't have a direct example except that it might help people with code organisation and reuse. I'll go with the simple implementation (no subcommands).

achilleas-k · 2024-05-28T11:30:13Z

doc/03-omnifest/02-external.md

+
+- `otk.external.foo` -> `otk_external_foo`
+- `otk.external.foo.bar_baz` -> `otk_external_foo bar_baz`
+- `otk.external.foo.bar.baz` -> `otk_external_foo bar baz`


Already I'm looking at this and thinking "is bar baz a single argument with a space or two args?"
If it's the former, how do I do two args?
If it's the latter, how do I do args with spaces?

Neither of these questions exist if the specification doesn't allow arguments in the otk.external directive's name and subcommands/arguments just become part of the input.

Yep, I'm going to drop it, it's confusing.

Still there?

Dropped, I think I only dropped it form the code implementation initially.

achilleas-k

LGTM generally. Tiny correction for a duplicate line.
Also, the subcommand/argument stuff and rephrase from my previous comments seem unchanged?

achilleas-k · 2024-06-03T14:12:49Z

doc/03-omnifest/02-external.md

+and the following directive:
+
+```yaml
+and the following directive:


Duplicate line snuck into the yaml code block.

achilleas-k · 2024-06-03T14:16:16Z

doc/03-omnifest/02-external.md

+
+- `otk.external.foo` -> `otk_external_foo`
+- `otk.external.foo.bar_baz` -> `otk_external_foo bar_baz`
+- `otk.external.foo.bar.baz` -> `otk_external_foo bar baz`


Still there?

achilleas-k

LGTM

mvo5

Thanks for the changes, the README looks very nice now! Fine to merge IMHO.

I have some questions in the implementation though, maybe I'm misunderstanding something but it seems like the parsing of external names to an executable and args for the executable was removed from the spec but the code seems to be still doing it? Is that because the otk_external_osbuild is easier to implement this way? i.e. instead of looking at "os.argv[0]" how it got called it would be called via the argument magic from "convert()"? If so we should reconsider if we want it back in the spec or if we want to change the code to look at argv[0] but it seems right now there is a bit of a mismatch between spec and code.

mvo5 · 2024-06-03T15:30:59Z

doc/03-omnifest/02-external.md

+When we have an [omnifest](./index.md) that looks like this:
+
+```yaml
+otk.external.name:
+  child:
+    - 1
+  options: "are here"
+```
+
+The external gets called with the following JSON:
+
+```json
+{
+  "tree": {
+    "otk.external.name": {
+      "child": [1],
+      "options": "are here"
+    }
+  }
+}
+```


Super nitpick - the initial part of the suggestion to change When we have an [omnifest](./index.md) that looks like this: -> Given the following [omnifest](https://github.com/osbuild/otk/pull/94/index.md): seems to be gotten lost (which is not a big deal but I wanted to mention it :)

mvo5 · 2024-06-03T15:35:53Z

doc/03-omnifest/02-external.md

+}
+```
+
+And is expected to return the following JSON:


Hm, this says "Done" but GH shows me the original version, is that intentional?

mvo5 · 2024-06-03T15:38:28Z

doc/03-omnifest/02-external.md

+}
+```
+
+Which will lead to the following YAML structure:


Similar to above, this says "Done" but AFAICT the text is unchanged, is it "done" in an indirect way?

mvo5 · 2024-06-03T15:45:26Z

src/otk/external.py

-        # TODO exception
-        sys.exit(1)
-        return
+    exe, args = convert(directive)


AFAICT we do not use "args" currently, maybe worth removing them until we need them? then this could become something like exe = exe_from_directive(directive) or exe = exe_from(directive)

src/otk/external.py

test/test_external.py

mvo5

Thanks for the update! I did a more careful review of the python code this time and noticed what appears to be a small oversight in the way the data from the external helper is loaded in call (hence the change request). My other comments are just ideas or suggestions :)

src/otk/external.py

test/test_external.py

src/otk/external.py

mvo5

Thanks for the updates! I went over it again and also updated the small integration test for the externals, I added checks in inputs too (sorry, forgot to do that yesterday). It's attached here:
0001-external-write-integration-test-for-the-happy-case.patch.txt

src/otk/external.py

doc/03-omnifest/02-external.md

achilleas-k · 2024-06-06T09:47:08Z

So this PR is getting a bit hard to re-re-review after all the comments and follow-up commits. Would it be difficult to have the commit history cleaned up a bit? For example, 062ed90 could be squashed into 8e172c9 (the commit that drops the usage of the OSBuildContext). Smaller commits like the 4 that follow the first can be squashed into the first one that's the bulk of the doc writing. Stuff like that.

Describes an initial external protocol. Signed-off-by: Simon de Vlieger <[email protected]>

Updates the externals to use a search path for executables instead of a registry of explicit conversions. Signed-off-by: Simon de Vlieger <[email protected]>

Rename `otk-osbuild` to fit in with the new specification. Signed-off-by: Simon de Vlieger <[email protected]>

mvo5

Thank you for the history cleanup! This looks pretty ready now, I have a few inline questions but those are no blockers so I'm fine approving if we want to do the open questions in a followup (happy to helper here of course :)

doc/03-omnifest/01-directive.md

src/otk/external.py

src/otk_external_osbuild/command.py

Remove unused arguments from the external calls, rename search to path_for to be more explicit about what the function does. Remove the contexts as they are documented to not be available. Also creates testcases for these functions. Signed-off-by: Simon de Vlieger <[email protected]>

The repetitive names with `otk_external` can instead be encoded by including an `external` in the search paths. Signed-off-by: Simon de Vlieger <[email protected]>

mvo5

Thank you!

supakeen linked an issue May 28, 2024 that may be closed by this pull request

Define the otk external protocol in documentation #87

Closed

mvo5 reviewed May 28, 2024

View reviewed changes

supakeen force-pushed the describe-external-protocol branch from 2091935 to 00cbe3a Compare May 28, 2024 10:37

supakeen changed the title ~~doc: describe external protocol~~ describe and implement the external protocol May 28, 2024

supakeen force-pushed the describe-external-protocol branch from d9333c8 to d3ca93d Compare May 28, 2024 10:52

achilleas-k reviewed May 28, 2024

View reviewed changes

mvo5 reviewed May 28, 2024

View reviewed changes

achilleas-k reviewed May 28, 2024

View reviewed changes

supakeen force-pushed the describe-external-protocol branch 3 times, most recently from 8b148cc to 65e7d5c Compare June 3, 2024 11:00

supakeen requested review from mvo5 and achilleas-k June 3, 2024 11:00

achilleas-k requested changes Jun 3, 2024

View reviewed changes

supakeen force-pushed the describe-external-protocol branch from 65e7d5c to bded701 Compare June 3, 2024 14:34

achilleas-k previously approved these changes Jun 3, 2024

View reviewed changes

mvo5 reviewed Jun 3, 2024

View reviewed changes

supakeen dismissed achilleas-k’s stale review via 5e647ac June 4, 2024 07:57

supakeen enabled auto-merge June 4, 2024 07:59

supakeen disabled auto-merge June 4, 2024 07:59

mvo5 requested changes Jun 4, 2024

View reviewed changes

supakeen force-pushed the describe-external-protocol branch from 90b7c3e to cd31fcd Compare June 4, 2024 11:42

mvo5 reviewed Jun 5, 2024

View reviewed changes

src/otk/external.py Show resolved Hide resolved

doc/03-omnifest/02-external.md Show resolved Hide resolved

supakeen force-pushed the describe-external-protocol branch from cd31fcd to 32212f8 Compare June 6, 2024 09:17

supakeen force-pushed the describe-external-protocol branch from 4442ecb to ef0ef83 Compare June 6, 2024 09:58

supakeen added 3 commits June 6, 2024 12:07

doc: describe external protocol

ef1861a

Describes an initial external protocol. Signed-off-by: Simon de Vlieger <[email protected]>

external: implementation

40fae66

Updates the externals to use a search path for executables instead of a registry of explicit conversions. Signed-off-by: Simon de Vlieger <[email protected]>

dist: rename otk-osbuild

0d9ae8d

Rename `otk-osbuild` to fit in with the new specification. Signed-off-by: Simon de Vlieger <[email protected]>

supakeen force-pushed the describe-external-protocol branch from ef0ef83 to 1863855 Compare June 6, 2024 10:08

mvo5 reviewed Jun 6, 2024

View reviewed changes

doc/03-omnifest/01-directive.md Outdated Show resolved Hide resolved

src/otk/external.py Outdated Show resolved Hide resolved

src/otk/external.py Outdated Show resolved Hide resolved

src/otk_external_osbuild/command.py Show resolved Hide resolved

supakeen added 2 commits June 7, 2024 11:22

external: search path and name

875aeb1

The repetitive names with `otk_external` can instead be encoded by including an `external` in the search paths. Signed-off-by: Simon de Vlieger <[email protected]>

supakeen force-pushed the describe-external-protocol branch from 1863855 to 875aeb1 Compare June 7, 2024 09:27

mvo5 approved these changes Jun 7, 2024

View reviewed changes

supakeen added this pull request to the merge queue Jun 7, 2024

Merged via the queue into osbuild:main with commit 66da377 Jun 7, 2024
1 check passed

supakeen mentioned this pull request Oct 2, 2024

external: Support multiple external paths #266

Merged

describe and implement the external protocol #94

describe and implement the external protocol #94

Conversation

supakeen commented May 28, 2024

mvo5 left a 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

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

mvo5 left a 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

mvo5 May 28, 2024 • 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

achilleas-k May 28, 2024 • 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

achilleas-k left a comment • 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

achilleas-k left a comment

Choose a reason for hiding this comment

mvo5 left a 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

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k commented Jun 6, 2024

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 May 28, 2024 •

edited

Loading

achilleas-k May 28, 2024 •

edited

Loading

achilleas-k left a comment •

edited

Loading