DocOps / liquidoc-gem

The canonical gem source for LiquiDoc, a ruby-based documentation parsing and rendering utility enabling advanced builds with Asciidoctor, Jekyll, Liquid, and semi-structured data files.
https://docs.docops.org/liquidoc-user-manual.html
MIT License
12 stars 5 forks source link

New 'execute' action type #65

Closed briandominick closed 5 years ago

briandominick commented 5 years ago

This change enables execution of arbitrary shell commands from within LiquiDoc, as discussed in Issue #64.

New functionality is documented in README.

For testing, try some commands such as:

- action: execute
  command: ls -l imports/product3/
  options:
    stdout: true
    outfile:
      path: _build/pre/products3_dirlist.stdout
    error:
      response: exit
      message: Make sure the directory got imported!
hpalacio commented 5 years ago

Hi @briandominick,

It works very well. Some comments: If the execute action is included in a loop, the log file gets overwritten by the last command. For example, I used it as a substitution of the migrate action in a loop like this:

{% for repo in repos %}
- action: execute
  command: echo {{repo}};cp -a ../{{repo}}/content/* _build/
  options:
    outfile:
      path: _build/pre/copy.stdout
{% endfor %}

and then I would only get the log of the last iteration. We could add yet another option to outfile like a boolean append, but I wonder if all these options make sense at all. I mean, for sure I want a build framework to log the results of the build somewhere so, why give the option at all? I would log the result of any exectute action (or any LiquiDoc action) to a pre-defined (or customizable) path. And then, similar to what Yocto does, assign an incremental ID per action and record the action details and the log in separate files. For instance, for a migrate action the tool would generate:

The next migrate action will generate migrate_2.run and migrate_2.log, and so on. Similarly, an execute action will generate execute_x.run with the details of the command, and execute_x.log with its log.

briandominick commented 5 years ago

I do have something of a logging system underway and some log-related issues in the queue, but I'd like to give it proper thought before I commit to a format. For these purposes, I was expecting to write to console or to a file, and I had never really considered generating a log file per-action.

I also don't see why you would not solve your output-file-overwrite problem with path: _build/pre/repo-{{repo}}-copy.stdout. Would this resolve the problem for now? I will give the logging system the attention it needs asap. See #66

hpalacio commented 5 years ago

I also don't see why you would not solve your output-file-overwrite problem with path: _build/pre/repo-{{repo}}-copy.stdout. Would this resolve the problem for now?

Yes, sure. It was a minor comment ;-)

Also, why to we need to have:

  options:
    outfile:
      path: _build/pre/copy.stdout

instead of simply:

  options:
    outfile: _build/pre/copy.stdout

Do you forsee additional parameters for the output file than just the path?

briandominick commented 5 years ago

Do you forsee additional parameters for the output file than just the path?

Yes, I even coded a couple already. I could see the need for a header row on top of an outfile, so I added prepend and append options. Too many diffs in the README (my bad), but here is the rundown.

I could change this so if outfile: is a string, we assume it's the path. If it's a struct, we read the params. I've done that elsewhere.

hpalacio commented 5 years ago

Ok. Just curious. For me it's fine.

hpalacio commented 5 years ago

@briandominick when do you plan to merge this?

briandominick commented 5 years ago

@hpalacio Have you tested it against your latest codebase? I've been trying to clean up some technical debt before doing another release but if you need this and have tested it, I'll merge and do a release. I think it's fully backward compatible don't think it should affect any existing LD codebase, but I have not released any upgrades without testing them against your stuff.

hpalacio commented 5 years ago

Hi @briandominick, yes I tested it and it works fine. I need it merged to work on other stuff. Thank you.