OTAVA-82: use ConfigArgParse to create Config

[smccarthy788]

See #82.

Took a stab at this. I fear I've misunderstood the goal:

use ConfigArgParse and support all options in the same library. (CLI options, yaml file, [env vars])

I've pushed the entire creation of the config object through ConfigArgParse. It feels a bit unwieldy, or... square peg through a round hole? So I'm not super attached to this implementation. Still, putting it up for consideration / reference.

Pros:

• all config options are available to be set in config file, env var, or CLI (with precedence as desired)
• good looking documentation of the CLI arguments & env vars output via otava --help:

Help Output

$ uv run otava --help
usage: otava [-h] [--config-file CONFIG_FILE] [--graphite-url GRAPHITE_URL] [--grafana-url GRAFANA_URL] [--grafana-user GRAFANA_USER] [--grafana-password GRAFANA_PASSWORD] [--slack-token SLACK_TOKEN] [--postgres-hostname POSTGRES_HOSTNAME]
             [--postgres-port POSTGRES_PORT] [--postgres-username POSTGRES_USERNAME] [--postgres-password POSTGRES_PASSWORD] [--postgres-database POSTGRES_DATABASE] [--bigquery-project BIGQUERY_PROJECT] [--bigquery-dataset BIGQUERY_DATASET]
             [--bigquery-credentials BIGQUERY_CREDENTIALS]
             {list-tests,list-metrics,list-groups,analyze,regressions,remove-annotations,validate} ...

Hunts performance regressions in Fallout results

positional arguments:
  {list-tests,list-metrics,list-groups,analyze,regressions,remove-annotations,validate}
    list-tests          list available tests
    list-metrics        list available metrics for a test
    list-groups         list available groups of tests
    analyze             analyze performance test results
    regressions         find performance regressions
    validate            validates the tests and metrics defined in the configuration

optional arguments:
  -h, --help            show this help message and exit
  --config-file CONFIG_FILE
                        Otava config file path [env var: OTAVA_CONFIG]

Graphite Options:
  Options for Graphite configuration

  --graphite-url GRAPHITE_URL
                        Graphite server URL [env var: GRAPHITE_ADDRESS]

Grafana Options:
  Options for Grafana configuration

  --grafana-url GRAFANA_URL
                        Grafana server URL [env var: GRAFANA_ADDRESS]
  --grafana-user GRAFANA_USER
                        Grafana server user [env var: GRAFANA_USER]
  --grafana-password GRAFANA_PASSWORD
                        Grafana server password [env var: GRAFANA_PASSWORD]

Slack Options:
  Options for Slack configuration

  --slack-token SLACK_TOKEN
                        Slack bot token to use for sending notifications [env var: SLACK_BOT_TOKEN]

Postgres Options:
  Options for Postgres configuration

  --postgres-hostname POSTGRES_HOSTNAME
                        PostgreSQL server hostname [env var: POSTGRES_HOSTNAME]
  --postgres-port POSTGRES_PORT
                        PostgreSQL server port [env var: POSTGRES_PORT]
  --postgres-username POSTGRES_USERNAME
                        PostgreSQL username [env var: POSTGRES_USERNAME]
  --postgres-password POSTGRES_PASSWORD
                        PostgreSQL password [env var: POSTGRES_PASSWORD]
  --postgres-database POSTGRES_DATABASE
                        PostgreSQL database name [env var: POSTGRES_DATABASE]

BigQuery Options:
  Options for BigQuery configuration

  --bigquery-project BIGQUERY_PROJECT
                        BigQuery project ID [env var: BIGQUERY_PROJECT_ID]
  --bigquery-dataset BIGQUERY_DATASET
                        BigQuery dataset [env var: BIGQUERY_DATASET]
  --bigquery-credentials BIGQUERY_CREDENTIALS
                        BigQuery credentials file [env var: BIGQUERY_VAULT_SECRET]

Args that start with '--' can also be set in a config file (specified via --config-file). Config file syntax allows: key=value, flag=true, stuff=[a,b,c] (for details, see syntax at https://goo.gl/R74nmi). In general, command-line values
override environment variables which override config file values which override defaults.

Cons:

• custom parser to handle nested config objects
• abstraction leaking into the config objects; config objects now need to know about configargparse.
• need to use parse_known_args to avoid exceptions from parts of the config we don't want to include in the CLI (tests, templates, test_groups)
• creating a Config object programatically from a configuration file is... weird (although I avoided exposing people to this by adding a helper function):

	config_file = Path("tests/resources/substitution_test_config.yaml")
    parser = create_config_parser(config_file)
    args, unknown_args = parser.parse_known_args(args=[])
    config = load_config_from_parser_args(args, config_file)

If we do want to pursue this implementation, I'll flesh out the unit tests more. So far I've only added enough to validate the basics.

[henrikingo]

Nice! This looks even better than I expected :-) But the ConfigArgParse library never failed me, so shouldn't be surprised.

For the nested settings, I would say don't be afraid to just flatten or otherwise change the structure of the config file. To me, that was kind of the point of doing the previous release, that now we have a release that is compatible with the past, and from here we can break compatibility if it makes Otava better in some ways.

I think there was something in the yaml configuration files - maybe it was the template system and how you configured each test... Where I felt that is probably best left as it is, "yaml only". I don't see it in this patch, so maybe that's because you likewise left those out of scope for this patch?

Btw, I never used Otava with these configuration files myself. I only ever saw you do it when I was at Datastax. (At Nyrkiö we use Otava as a python library.)

[henrikingo]

Actually it is common to import configargparse as argparse to avoid having to change the rest of the code.

[smccarthy788]

For the nested settings, I would say don't be afraid to just flatten or otherwise change the structure of the config file.

Personally I quite like the nested structure of the config. IMO it's worth the custom parser.

I think there was something in the yaml configuration files - maybe it was the template system and how you configured each test... Where I felt that is probably best left as it is, "yaml only". I don't see it in this patch, so maybe that's because you likewise left those out of scope for this patch?

I haven't touched how the tests are generated from the templates (I think this is what you're referring to). Still "yaml only", even more so now that I've removed the expandvars (no more arbitrary env vars).

I have an itch to separate those from the config, but that would probably be best as a separate PR.

[smccarthy788]

@henrikingo I think this is ready for another round of review. @Gerrrr I'd appreciate your feedback too.

[henrikingo]

+1

If @Gerrrr doesn't reply soon just merge since this fixes pretty annoying behavior

[Gerrrr]

I will only be able to review this PR closer to the end of next week. Since @henrikingo reviewed it, there is no need to wait for me.

[henrikingo]

It's allowed to review also after merging. But this is fairly straightforward adoption of configargparse. Good work Sean.