-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add generators #52
Add generators #52
Conversation
77df6cf
to
2626791
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for putting this on the back burner for so long. I have focused on reading the implementation parts (rather than the test parts) for now. In general, this looks pretty good so far. Besides the more specific nitpicks down below, I have one big gripe: The output format for generators deviates significantly from that for entities, even though it could be extremely similar. For example:
$ holo scan
Running generator example.sh
found at /usr/share/holo/generators/example.sh
ls: /foo: no such file or directory
!! exit status 1
or even just
$ holo scan
Running generator /usr/share/holo/generators/example.sh
ls: /foo: no such file or directory
!! exit status 1
Like with entities, the prologue paragraph ("Running generator...") would only be shown if the generator produces stdout/stderr or a non-zero exit status, so during normal operation this would be entirely silent. (When no errors occur, holo apply
only reports when system configuration actually gets touched, and holo scan
only lists entities.)
Please consider this change. It would make generators feel much more like first-class citizens.
env := os.Environ() | ||
env = append( | ||
env, | ||
fmt.Sprintf("OUT=%s", targetDir), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generators should also get HOLO_RESOURCE_DIR
and HOLO_CACHE_DIR
to ensure that they don't hardcode /usr/share/holo
or /tmp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HOLO_RESOURCE_DIR
is usually adjusted to be plugin specific. What would be the content of it for generators? Correct me if I am wrong, but isn't /use/share/holo
currently hard coded in plugin.go? In other words the plugins are flexible to where their resource files live, but holo isn't. We should potentially introduce a HOLO_RESOURCE_ROOT
.
} | ||
|
||
func getGeneratorCacheDir() (string, error) { | ||
path, err := prepareDir(RootDirectory(), "/var/tmp/holo/generated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since generated resource files are extremely transient, /tmp
looks to be more appropriate than /var/tmp
. I'd prefer having all generated files inside $HOLO_CACHE_DIR
(like in the fallback path of this method) to simplify cleanup and to ensure that we don't accidentally reuse old files from a previous run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted generated files to be cacheable (let the generator decide whether to regenerate or not) and avoid excessive regeneration. But thinking about it this leads to a problem when a generator just stops to generate a certain file it previously generated. Would you say we leave it to the generator to maintain his own cache and just always clean the generated files?
Generators are executed before the scan phase and can generated file for other plugins.
This has obviously been on my list for quite a while now, and upon first looking at the diff, I wondered why I kept putting this off for so long. But once I got into it, I found that my anticipation of dread was correct. I was hoping to get this feature done and shipped in a single evening, but it's 1AM now and I need to get some rest. My current progress is on the |
I see two alternatives to actually copying the files:
Just throwing these out there in case you haven't considered them yet. Just to clarify: The only thing missing is to adjust the tests to the new implementation. Is that correct? Another thing I wanted to mention: Do you see any way to keep a persistant cache for generators? Since generators are executed at every run of holo it would be good to avoid unnecessary regeneration of the same files in order to reduce execution time. |
The cache dir is in
My primary concern with this feature ist that it increases the complexity of Holo's execution model quite a bit, so I'm actively trying to avoid adding extra complexity. As you said, working with symlinks brings lots of extra complexity and extra gotchas, so I would like to not do that unless absolutely necessary.
Much appreciated. I'm fairly positive that I will have considered all consequences and gotchas with this feature by the time I merge it, though.
For the most part, yes. I also want to extend CLI selectors to generated resource files. Right now, you can say
I removed that part from the spec and implementation for the same reason as above, to reduce complexity both in implementation and mental model. If caching saves half a second, but increases the chance of bugs and gotchas, I'll go without caching. To witness, EDIT: I just realized that a persistent cache would introduce two other problems. First, it would mean that |
I'm sorry it took so long but I've finally finished an intial version of the generator feature.
Generators allow dynamically generating files for plugins. They are executable files placed under
/usr/share/holo/generators
. They get passed the environment variable$OUT
containing a folder to which they can write generated files. Plugins will then pick up this files as if they were placed under/usr/share/holo/
.Fixes #51