search

danb35 / deploy-freenas

Python script to automate deploying TLS certificates to FreeNAS servers
GNU General Public License v3.0
203 stars 56 forks source link

Argparse #8

Closed mt7479 closed 6 years ago

mt7479 commented 6 years ago

I added command line options, this would also fix #5. Not sure if you are interested in having that but I thought I send you the pr anyway.

danb35 commented 6 years ago

Thanks for the suggestion, and the work on implementing it. Not being familiar with argparse, is it possible to read values from a file as well as/instead of the command line? I ask because I envision this script primarily being called by an ACME client (specifically, acme.sh, though it could be any other client), and needing to specify everything on the command line could make for a pretty unwieldy command when the cert is issued.

mt7479 commented 6 years ago

It could probably be done with mutual exclusive parameters and following sub parameters for the non-config file approach. However i'm not sure if it is possible to assign a custom variable name for a parameter instead of args.foo. A quick search came up with nothing.

Given as it is, it might make more sense to abandon my arguments approach and implement a config file. My motivation is to keep in sync with your upstream changes without copy/pasting my config options over and over.

danb35 commented 6 years ago

Yes, and I agree with your motivation--that's why I coded my Nextcloud script to use a config file. And though I'm sure it's trivially easy to do it, I'm not quite sure how to read in a config file in Python. But I should look into it.

danb35 commented 6 years ago

I'm working on a branch to use configparser to read a config file. Take a look at the use-config-file branch if you're interested. Very much a work in progress at the moment.

mt7479 commented 6 years ago

I wanted to suggest configparser myself and offer to look into in the weekend but you beat me to it :smile:

Looks fine to me. A small suggestion though, argparse could be used to a add an optional config file location parameter, using your deploy_config as default but allow overriding the location if needed.

I could probably add that in a timely fashion if you see any use in it.

danb35 commented 6 years ago

I've tested the changes using configparser, they work, so I've merged them. I could see, though, how the ability to specify the location of the config file could be handy--I'd be interested in taking a look at a PR for that.

mt7479 commented 6 years ago

Ok, I will add this today. Should I add `os.path.isfile? to check if the file path is valid and display a friendly error message ?

mt7479 commented 6 years ago

Opened a new pull request, let's discuss there.

12