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

Support YAML configs #262

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Support YAML configs #262

merged 1 commit into from
Aug 25, 2022

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Aug 24, 2022

Which problem is this PR solving?

Short description of the changes

It turns out that the library we use for parsing command lines and .INI configuration files has a bug where it just doesn't properly support multi-valued fields in INI files. The bug has existed for a while and the repository is stale.

I could have forked the library, fixed the bug, and then we'd have to maintain a fork forever since it seems unlikely to ever be fixed upstream.

Or I could have tried to switch to some other argument parser, but that would almost certainly have a whole collection of backwards-compatibility issues.

In addition, ini files are also pretty stale as a general concept. It seemed worthwhile to support a more modern format, and YAML is certainly eating the world as a config file format.

So I've done this:

  • Chosen a popular yaml parser
  • Decorated all of the configuration options with appropriate YAML tags to make sure that the configuration names are preserved
  • Added a flag to write out a YAML config after reading the existing config (so that transitioning to the new format is easy)
  • Added a flag to load a YAML config

Note that in the INI file world, because the config parser and the command line parser were the same entity, individual values in the config file could be overridden on the command line. But if you load a YAML file, all other config options are ignored.

No .INI files have been harmed in the making of this PR. All existing .INI behavior has been preserved -- this just lets us deprecate them and move to YAML instead.

The Honeycomb documentation also needs to be updated. I'll put up a PR for that soon.

@kentquirk kentquirk requested review from a team and pkanal August 24, 2022 23:36
@porterctrlz
Copy link

Bee still my beating heart :)

Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I've left one suggestion to add an empty yaml declaration for Fields in the csv options struct.

@@ -17,10 +17,11 @@ import (
// Options defines the options relevant to the CSV parser
type Options struct {
Fields string `long:"fields" description:"Comma separated list of CSV fields, in order."`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a yam:"-" for Fields too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely not "-" -- that would prevent the field from ever showing up. The rule I used was to add a YAML declaration only when the field name used in config didn't match the variable name, or when it should be suppressed. In this case, the field is called "Fields" and that's also the config value name, so a YAML tag naming the field would be redundant.

@kentquirk kentquirk merged commit 212bc84 into main Aug 25, 2022
@kentquirk kentquirk deleted the kent.261 branch August 25, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants