Skip to content
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

replace the plugin command line protocol with plugin.yaml and environment variables #31

Closed
oconnor663 opened this issue Aug 1, 2014 · 4 comments

Comments

@oconnor663
Copy link
Member

Right now plugins have to do some nontrivial parsing to separate out plugin fields from command arguments. This gets duplicated in every plugin, even though some of it is shared. A fairly trivial plugin like cp, which should be one line, ends up being four or five (let along the Bash rsync plugin), and also the sets of mandatory and optional fields get duplicated between fetch and reup scripts.

One of the reasons we didn't use more environment variables earlier is that it's difficult for the plugin to recognize invalid fields if it doesn't its fields in a list. But it shouldn't be the plugin's responsibility to recognize invalid fields -- that's more duplicated logic that should live in peru core. We should create a plugin.yaml convention that lets the plugin declare what fields it supports. (And possibly other stuff in the future, who knows.)

Once that's done, there's no reason not to pass the url field as e.g. PERU_FIELD_URL or something. Then the plugin never needs to parse anything.

@olson-sean-k olson-sean-k changed the title Replace the plugin command line protocol with plugin.yaml and environment variables replace the plugin command line protocol with plugin.yaml and environment variables Aug 1, 2014
@olson-sean-k
Copy link
Member

I'm still a tad wary of this approach. If we export variables from peru, then won't we need to worry about cleaning that state (i.e., flush/write/unset all relevant variables) before each call to a plugin? It could be easy to miss something. Will this work equally well on Windows as it does on Linux?

I totally agree that validation really ought not live in plugins; they should trust that peru has correctly processed and scrubbed their parameters, and metadata in plugin.yaml should facilitate that.

@olson-sean-k
Copy link
Member

I guess subprocess accepts a dict of environment variables, so this should fine.

@oconnor663
Copy link
Member Author

Right. Also I think we want the subprocess to inherit random variables from the parent, in case there are global hacks ($LD_PRELOAD magic) that should apply to the plugin as well as the parent. But it might make sense to scrub the variables that we know are immediately relevant, like anything starting with our PERU_FIELD_ prefix or whatever it ends up being.

@olson-sean-k
Copy link
Member

Here's a diff for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants