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

Add dry-run option to baseplate-script #56

Closed
wants to merge 3 commits into from

Conversation

pacejackson
Copy link
Contributor

No description provided.

@pacejackson
Copy link
Contributor Author

👓 @spladug

How do you feel about this way of communicating dry-run vs live mode? Personally, I would like to make dry-run an argument that get's passed to the function baseplate-script calls but then anyone using this functionality would have to update their scripts to accept it.

@spladug
Copy link
Contributor

spladug commented Nov 3, 2016

I'm not against modifying the signature of the function to be called; we're still in semi-unstable mode here and baseplate-script isn't used heavily much. However, I think this particular flag is probably too specific to certain scripts since not everything needs a dry run flag.

Instead, it hints at a general need to allow scripts to parse their own arguments; would we add another argument to baseplate-script when we want to pass e.g. hostname?

As an alternative, what about making load_and_run_script do ArgumentParser.parse_known_args and then altering the expected script function's signature to take the remaining arguments as a parameter? The called script can ignore them or run its own argument parser on them if desired. This does have the downside that --help won't carry any of the script's own argument parsing info AFAIK, but I'm not sure if there's something better since it seems difficult to parse arguments, find the script to run, load it, let it modify the argument parser, then parse arguments again.

💅

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

Successfully merging this pull request may close these issues.

None yet

2 participants