Skip to content

Make CSV importer configurable via ConfigArgParse #166

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.

Sign up for GitHub

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

Open
MrlixiangWE wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Make CSV importer configurable via ConfigArgParse #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70fdcad
Make CSV importer configurable via ConfigArgParse
MrlixiangWE Aug 4, 2026
253a963
Keep quotechar for CSV YAML configuration
MrlixiangWE Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions docs/CSV.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,22 @@ tests:
quotechar: "'"
```

## CSV options

The delimiter and quote character can also be configured globally:

```yaml
csv:
delimiter: ';'
quotechar: "'"
```

The corresponding command-line options are `--csv-delimiter` and
`--csv-quote-char`. They can also be set through the `CSV_DELIMITER` and
`CSV_QUOTE_CHAR` environment variables. Explicit global values override the
`csv_options` values of individual tests; without a global value, existing
per-test settings and defaults are unchanged.

## Example

```bash
Expand Down
2 changes: 1 addition & 1 deletion docs/GETTING_STARTED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ tests:
attributes: [commit]
csv_options:
delimiter: ","
quote_char: "'"
quotechar: "'"
```

The `time_column` property points to the name of the column storing the timestamp
Expand Down
25 changes: 21 additions & 4 deletions otava/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from ruamel.yaml import YAML

from otava.bigquery import BigQueryConfig
from otava.csv_options import CsvConfig
from otava.grafana import GrafanaConfig
from otava.graphite import GraphiteConfig
from otava.postgres import PostgresConfig
Expand All @@ -33,6 +34,7 @@

@dataclass
class Config:
csv: CsvConfig
graphite: Optional[GraphiteConfig]
grafana: Optional[GrafanaConfig]
tests: Dict[str, TestConfig]
Expand All @@ -54,7 +56,9 @@ def load_templates(config: Dict) -> Dict[str, Dict]:
return templates


def load_tests(config: Dict, templates: Dict) -> Dict[str, TestConfig]:
def load_tests(
config: Dict, templates: Dict, csv_config: Optional[CsvConfig] = None
) -> Dict[str, TestConfig]:
tests = config.get("tests", {})
if not isinstance(tests, Dict):
raise ConfigError("Property `tests` is not a dictionary")
Expand All @@ -69,7 +73,7 @@ def load_tests(config: Dict, templates: Dict) -> Dict[str, TestConfig]:
except KeyError as e:
raise ConfigError(f"Template {e.args[0]} referenced in test {test_name} not found")
test_config = merge_dict_list(template_list + [test_config])
result[test_name] = create_test_config(test_name, test_config)
result[test_name] = create_test_config(test_name, test_config, csv_config)

return result

Expand All @@ -96,20 +100,22 @@ def load_test_groups(config: Dict, tests: Dict[str, TestConfig]) -> Dict[str, Li


def load_config_from_parser_args(args: configargparse.Namespace) -> Config:
csv_config = CsvConfig.from_parser_args(args)
config_file = getattr(args, "config_file", None)
if config_file is not None:
yaml = YAML(typ="safe")
config = yaml.load(Path(config_file).read_text())

templates = load_templates(config)
tests = load_tests(config, templates)
tests = load_tests(config, templates, csv_config)
groups = load_test_groups(config, tests)
else:
logging.warning("Otava configuration file not found or not specified")
tests = {}
groups = {}

return Config(
csv=csv_config,
graphite=GraphiteConfig.from_parser_args(args),
grafana=GrafanaConfig.from_parser_args(args),
slack=SlackConfig.from_parser_args(args),
Expand All @@ -128,13 +134,18 @@ class NestedYAMLConfigFileParser(configargparse.ConfigFileParser):
"""

CLI_CONFIG_SECTIONS = [
CsvConfig.NAME,
GraphiteConfig.NAME,
GrafanaConfig.NAME,
SlackConfig.NAME,
PostgresConfig.NAME,
BigQueryConfig.NAME,
]

CONFIG_KEY_ALIASES = {

@Gerrrr Gerrrr Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really on the fence with CONFIG_KEY_ALIASES. @MrlixiangWE can you please elaborate your thoughts on why do we want to introduce them?

My take is, if we really dislike quotechar - now is the time to break backwards compatibility and rename it to quote-char everywhere (although, I'd like to know the justification for it). OTOH if we can live with quotechar, why don't we just add csv-quotechar and call it a day?

@MrlixiangWE MrlixiangWE Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alias table goes away either way — it was bridging a divergence this PR itself created. But let me give the justification you asked for, which I should have led with in the first round instead of just renaming things.

Across the config surface, multi-word names are separated: time_column, csv_options, update_statement, test_groups, base_branch, bigquery.project_id in YAML; --bigquery-project-id, --since-commit, --config-file on the CLI; BIGQUERY_PROJECT_ID and friends in the environment. quotechar is the one concatenated exception (not counting hostname and username, which are ordinary words). It comes from Python's csv API, but someone writing an otava config shouldn't need to know that — it's just one more special spelling to memorize. That's what the original rename was about — and it matched the names the code itself already used (CsvOptions.quote_char, and the per-test loader key). When you asked to keep the existing name, I took that to mean the per-test files specifically, restored quotechar there, and named the new flag --csv-quote-char to match the other options — then papered over the gap with the alias. In hindsight I should have raised the naming question openly instead of doing it in halves.

One thing worth knowing before we decide: on current master the per-test loader reads only quote_char:

otava/otava/test_config.py

Lines 257 to 259 in cd5bc1e

if test_info.get("csv_options"):
csv_options.delimiter = test_info["csv_options"].get("delimiter", ",")
csv_options.quote_char = test_info["csv_options"].get("quote_char", '"')

so the quotechar in docs/CSV.md and examples/csv/config/otava.yaml is silently ignored today — the example never notices because its CSV has no quoted fields. The docs and the code already disagree, and a full rename to quote_char wouldn't break any config that actually works.

So my vote is the deliberate rename: quote_char in YAML, --csv-quote-char, CSV_QUOTE_CHAR, docs and examples updated to match, no aliases and no fallback. If you'd rather keep quotechar, I'll do that instead — --csv-quotechar and done. Either way the PR ends with exactly one public spelling.

"csv-quotechar": "csv-quote-char",
}

def parse(self, stream):
yaml = YAML(typ="safe")
config_data = yaml.load(stream)
Expand All @@ -147,6 +158,11 @@ def parse(self, stream):
# Flatten only the config sections that correspond to CLI arguments
self._flatten_dict(value, flattened_dict, f"{key}-")
# Ignore other sections like 'templates' and 'tests' - they shouldn't become CLI arguments

for source, target in self.CONFIG_KEY_ALIASES.items():
if source in flattened_dict:
flattened_dict[target] = flattened_dict.pop(source)

return flattened_dict

def _flatten_dict(self, nested_dict, flattened_dict, prefix=''):
Expand Down Expand Up @@ -174,7 +190,8 @@ def get_syntax_description(self):


def add_service_option_groups(parser) -> None:
"""Add Graphite, Grafana, Slack, Postgres, and BigQuery option groups to a parser."""
"""Add importer and integration option groups to a parser."""
CsvConfig.add_parser_args(parser.add_argument_group('CSV Options', 'Options for CSV configuration'))
GraphiteConfig.add_parser_args(parser.add_argument_group('Graphite Options', 'Options for Graphite configuration'))
GrafanaConfig.add_parser_args(parser.add_argument_group('Grafana Options', 'Options for Grafana configuration'))
SlackConfig.add_parser_args(parser.add_argument_group('Slack Options', 'Options for Slack configuration'))
Expand Down
33 changes: 33 additions & 0 deletions otava/csv_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,39 @@

import enum
from dataclasses import dataclass
from typing import Optional

import configargparse


@dataclass
class CsvConfig:
NAME = "csv"

delimiter: Optional[str] = None
quote_char: Optional[str] = None

@staticmethod
def add_parser_args(arg_group):
arg_group.add_argument(
"--csv-delimiter",
help="CSV delimiter",
env_var="CSV_DELIMITER",
default=configargparse.SUPPRESS,
)
arg_group.add_argument(
"--csv-quote-char",
help="CSV quote character",
env_var="CSV_QUOTE_CHAR",
default=configargparse.SUPPRESS,
)

@staticmethod
def from_parser_args(args):
return CsvConfig(
delimiter=getattr(args, "csv_delimiter", None),
quote_char=getattr(args, "csv_quote_char", None),
)


@dataclass
Expand Down
24 changes: 18 additions & 6 deletions otava/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from dataclasses import dataclass
from typing import Dict, List, Optional

from otava.csv_options import CsvOptions
from otava.csv_options import CsvConfig, CsvOptions


@dataclass
Expand Down Expand Up @@ -199,7 +199,9 @@ def fully_qualified_metric_names(self) -> List[str]:
return list(self.metrics.keys())


def create_test_config(name: str, config: Dict) -> TestConfig:
def create_test_config(
name: str, config: Dict, csv_config: Optional[CsvConfig] = None
) -> TestConfig:
"""
Loads properties of a test from a dictionary read from otava's config file
This dictionary must have the `type` property to determine the type of the test.
Expand All @@ -208,7 +210,7 @@ def create_test_config(name: str, config: Dict) -> TestConfig:
"""
test_type = config.get("type")
if test_type == "csv":
return create_csv_test_config(name, config)
return create_csv_test_config(name, config, csv_config)
elif test_type == "graphite":
return create_graphite_test_config(name, config)
elif test_type == "histostat":
Expand All @@ -225,7 +227,9 @@ def create_test_config(name: str, config: Dict) -> TestConfig:
raise TestConfigError(f"Unknown test type {test_type} for test {name}")


def create_csv_test_config(test_name: str, test_info: Dict) -> CsvTestConfig:
def create_csv_test_config(
test_name: str, test_info: Dict, csv_config: Optional[CsvConfig] = None
) -> CsvTestConfig:
csv_options = CsvOptions()
try:
file = test_info["file"]
Expand Down Expand Up @@ -255,8 +259,16 @@ def create_csv_test_config(test_name: str, test_info: Dict) -> CsvTestConfig:
raise TestConfigError(f"Attributes of the test {test_name} must be a list")

if test_info.get("csv_options"):
csv_options.delimiter = test_info["csv_options"].get("delimiter", ",")
csv_options.quote_char = test_info["csv_options"].get("quote_char", '"')
per_test_options = test_info["csv_options"]
csv_options.delimiter = per_test_options.get("delimiter", ",")
csv_options.quote_char = per_test_options.get(
"quotechar", per_test_options.get("quote_char", '"')
)
if csv_config is not None:
if csv_config.delimiter is not None:
csv_options.delimiter = csv_config.delimiter
if csv_config.quote_char is not None:
csv_options.quote_char = csv_config.quote_char
return CsvTestConfig(
test_name,
file,
Expand Down
Loading