Aca-py startup arg updates #739

ianco · 2020-10-02T13:29:49Z

Make "--endpoint" required. Make genesis required (url, file or txns) unless "--no-ledger" is specified. Make some "indy" wallet parameters required (name, key, etc) if indy wallet type is specified.

Added "--arg-file " to allow params to be included in a file. (File is just a list of args, not structured with yml etc) Had to change some related logic - couldn't flag args as "required" so moved the check to the "get_settings()" method and threw "ArgsParseError"; display usage on any ArgsParseError. (Had to fix a couple of unit tests that expected the "old" behaviour.

Related to issue #521

Signed-off-by: Ian Costanzo <[email protected]>

swcurran

Nice!

Signed-off-by: Ian Costanzo <[email protected]>

codecov-commenter · 2020-10-02T22:28:30Z

Codecov Report

Merging #739 into master will decrease coverage by 0.02%.
The diff coverage is 72.09%.

@@            Coverage Diff             @@
##           master     #739      +/-   ##
==========================================
- Coverage   99.01%   98.99%   -0.03%     
==========================================
  Files         256      256              
  Lines       13956    13975      +19     
==========================================
+ Hits        13819    13835      +16     
- Misses        137      140       +3

swcurran · 2020-10-02T22:29:28Z

Hmmm... -0.03% takes us below 99%. That's sad...

ianco · 2020-10-02T22:36:02Z

Note that configargparse is pointing to my fork (in requirements.txt) pending them accepting my PR

Signed-off-by: Ian Costanzo <[email protected]>

…s wallet Signed-off-by: Ian Costanzo <[email protected]>

Signed-off-by: Ian Costanzo <[email protected]>

ianco · 2020-10-05T19:53:44Z

Some discussion around environment variables - the "ConfigArgParse" library adds support for env vars, but they must be configured per parameter, like for example:

    parser.add_argument("--wallet-storage-config", env_var="ACAPY_WALLET_STORAGE_CONFIG", help='....')

If the environment variable is present the value gets used, regardless of whether there is a config file or not.

However you cannot substitute env vars in the config file, like the following:

    input-transport = [["http", "${HOST}", "${PORT}"]]

The list of available environment variables (from the parser.add_argument() functions) is included in the usage() ("aca-py -h"), like:

--wallet-rekey
Specifies .... [env var: ACAPY_WALLET_REKEY]

Signed-off-by: Ian Costanzo <[email protected]>

swcurran · 2020-10-05T19:59:31Z

Some discussion around environment variables - the "ConfigArgParse" library adds support for env vars, but they must be configured per parameter, like for example:
    parser.add_argument("--wallet-storage-config", env_var="ACAPY_WALLET_STORAGE_CONFIG", help='....')
If the environment variable is present the value gets used, regardless of whether there is a config file or not.

However you cannot substitute env vars in the config file, like the following:
    input-transport = [["http", "${HOST}", "${PORT}"]]
The list of available environment variables (from the parser.add_argument() functions) is included in the usage() ("aca-py -h"), like:

--wallet-rekey
Specifies .... [env var: ACAPY_WALLET_REKEY]

IMHO - this is even better. We get DRY documentation, we tightly tie the environment variables to the arguments. It does change how we have to environment variables, but the change looks like it moves us in the right direction. Ideal to use this always.

Next challenge -- being able to use argparse as defined here in bash scripts :-) -- that would be awesome...

ianco · 2020-10-05T20:06:33Z

It's not perfect - it would be nice to have the param substitution within the config file as well.

For example multi-value parameters (like "input-transport", which is also a multi-instance parameter = multi instances of multi values) would be confusing to jam into a single environment variable.

swcurran · 2020-10-05T20:11:28Z

Except there is a very clear rule on how to do it, with usage information that makes it precise. What we are doing now is mixing and matching approaches that vary from variable to variable. Neither is perfect, but I would lean to consistency.

But my vote doesn't count much (and rightly so :-) ) in these decisions.

ianco · 2020-10-06T18:06:19Z

Env vars configured for:

--admin-api-key: ACAPY_ADMIN_API_KEY
--webhook-url: ACAPY_WEBHOOK_URL
--seed: ACAPY_WALLET_SEED
--wallet-key: ACAPY_WALLET_KEY
--wallet-rekey: ACAPY_WALLET_REKEY
--wallet-storage-config: ACAPY_WALLET_STORAGE_CONFIG
--wallet-storage-creds: ACAPY_WALLET_STORAGE_CREDS

esune · 2020-10-06T18:17:37Z

--admin-api-key: ACAPY_ADMIN_API_KEY
--webhook-url: ACAPY_WEBHOOK_URL
--seed: ACAPY_WALLET_SEED
--wallet-key: ACAPY_WALLET_KEY
--wallet-rekey: ACAPY_WALLET_REKEY
--wallet-storage-config: ACAPY_WALLET_STORAGE_CONFIG
--wallet-storage-creds: ACAPY_WALLET_STORAGE_CREDS

Additional settings that we will likely need an environment variables for:
--label
--tails-server-base-url
--wallet-type
--wallet-storage-type
--wallet-storage-creds
--wallet-storage-config
--endpoint

If it was possible to use an environment variable to drive whether the agent starts in insecure mode or uses an api-key it would be good, but maybe out of scope now (this could also be tied to whether the admin interface is activated or not - I can't think of a use for an agent that has no admin interface).

In general I would think having environment variables for all - or at least as many as possible - of the settings would be a good idea, both for consistency and especially when working with cloud deployments of any kind, where they are the easiest way to deal with configurations. At the same time, I realize that some of the environment variables (see the api-key/admin interface example above) would potentially force us to build additional logic to check whether other environment variables are set based on their value. This is a bigger job than we maybe willing to do now, but will definitely make the agents more user-friendly and resilient.

Signed-off-by: Ian Costanzo <[email protected]>

ianco · 2020-10-06T22:24:38Z

--admin-api-key: ACAPY_ADMIN_API_KEY
--webhook-url: ACAPY_WEBHOOK_URL
--seed: ACAPY_WALLET_SEED
--wallet-key: ACAPY_WALLET_KEY
--wallet-rekey: ACAPY_WALLET_REKEY
--wallet-storage-config: ACAPY_WALLET_STORAGE_CONFIG
--wallet-storage-creds: ACAPY_WALLET_STORAGE_CREDS

Additional settings that we will likely need an environment variables for:
--label
--tails-server-base-url
--wallet-type
--wallet-storage-type
--wallet-storage-creds
--wallet-storage-config
--endpoint

If it was possible to use an environment variable to drive whether the agent starts in insecure mode or uses an api-key it would be good, but maybe out of scope now (this could also be tied to whether the admin interface is activated or not - I can't think of a use for an agent that has no admin interface).

In general I would think having environment variables for all - or at least as many as possible - of the settings would be a good idea, both for consistency and especially when working with cloud deployments of any kind, where they are the easiest way to deal with configurations. At the same time, I realize that some of the environment variables (see the api-key/admin interface example above) would potentially force us to build additional logic to check whether other environment variables are set based on their value. This is a bigger job than we maybe willing to do now, but will definitely make the agents more user-friendly and resilient.

Added

codecov-io · 2020-10-06T22:25:54Z

Codecov Report

❗ No coverage uploaded for pull request base (master@015928f). Click here to learn what that means.
The diff coverage is 72.09%.

@@            Coverage Diff            @@
##             master     #739   +/-   ##
=========================================
  Coverage          ?   98.99%           
=========================================
  Files             ?      256           
  Lines             ?    13975           
  Branches          ?        0           
=========================================
  Hits              ?    13835           
  Misses            ?      140           
  Partials          ?        0

esune

Looks good! Thanks @ianco

esune · 2020-10-07T00:01:52Z

requirements.txt

+py_multicodec==0.2.1
+git+https://github.com/ianco/ConfigArgParse.git#egg=ConfigArgParse


Should we log an issue to remember updating this dependency, when you PR is merged?

WadeBarnes

Looks good!

ianco added 2 commits October 1, 2020 15:21

Tweak some of the required startup params

7b7fdad

Signed-off-by: Ian Costanzo <[email protected]>

Add support for file-based args

ff33699

Signed-off-by: Ian Costanzo <[email protected]>

swcurran reviewed Oct 2, 2020

View reviewed changes

ianco added 2 commits October 2, 2020 13:17

Merge remote-tracking branch 'upstream/master' into args_updates

892097c

Update to use configargparse for file-based config

578d49d

Signed-off-by: Ian Costanzo <[email protected]>

ianco marked this pull request as ready for review October 2, 2020 22:35

ianco added 5 commits October 2, 2020 15:48

Formatting updates

c0d8e6e

Signed-off-by: Ian Costanzo <[email protected]>

Remove git dependency in setup script

c4dec71

Signed-off-by: Ian Costanzo <[email protected]>

Example of a config file to connect to local indy network and postgre…

d9f95bf

…s wallet Signed-off-by: Ian Costanzo <[email protected]>

Add env var support to the demo

b4c3379

Signed-off-by: Ian Costanzo <[email protected]>

Tweak demo config file

442e115

Signed-off-by: Ian Costanzo <[email protected]>

ianco requested review from sklump, andrewwhitehead and WadeBarnes October 5, 2020 19:50

Fix env var name

0084612

Signed-off-by: Ian Costanzo <[email protected]>

ianco added 2 commits October 6, 2020 14:43

Add env vars for all params

58989b0

Signed-off-by: Ian Costanzo <[email protected]>

Fix default behaviour for storage type

2b6fe81

Signed-off-by: Ian Costanzo <[email protected]>

esune approved these changes Oct 7, 2020

View reviewed changes

WadeBarnes approved these changes Oct 7, 2020

View reviewed changes

andrewwhitehead merged commit 1f9a3bc into openwallet-foundation:master Oct 7, 2020

swcurran mentioned this pull request Oct 8, 2020

Add test to automate the detection/documentation of Admin API changes #742

Closed

ianco deleted the args_updates branch December 2, 2020 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aca-py startup arg updates #739

Aca-py startup arg updates #739

ianco commented Oct 2, 2020 •

edited

Loading

swcurran left a comment

codecov-commenter commented Oct 2, 2020

swcurran commented Oct 2, 2020

ianco commented Oct 2, 2020

ianco commented Oct 5, 2020 •

edited

Loading

swcurran commented Oct 5, 2020

ianco commented Oct 5, 2020

swcurran commented Oct 5, 2020

ianco commented Oct 6, 2020

esune commented Oct 6, 2020 •

edited

Loading

ianco commented Oct 6, 2020

codecov-io commented Oct 6, 2020

esune left a comment

esune Oct 7, 2020

WadeBarnes left a comment

		py_multicodec==0.2.1
		git+https://github.com/ianco/ConfigArgParse.git#egg=ConfigArgParse

Aca-py startup arg updates #739

Aca-py startup arg updates #739

Conversation

ianco commented Oct 2, 2020 • edited Loading

swcurran left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 2, 2020

Codecov Report

swcurran commented Oct 2, 2020

ianco commented Oct 2, 2020

ianco commented Oct 5, 2020 • edited Loading

swcurran commented Oct 5, 2020

ianco commented Oct 5, 2020

swcurran commented Oct 5, 2020

ianco commented Oct 6, 2020

esune commented Oct 6, 2020 • edited Loading

ianco commented Oct 6, 2020

codecov-io commented Oct 6, 2020

Codecov Report

esune left a comment

Choose a reason for hiding this comment

esune Oct 7, 2020

Choose a reason for hiding this comment

WadeBarnes left a comment

Choose a reason for hiding this comment

ianco commented Oct 2, 2020 •

edited

Loading

ianco commented Oct 5, 2020 •

edited

Loading

esune commented Oct 6, 2020 •

edited

Loading