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

YAML support for the file backend #339

Merged
merged 4 commits into from
Oct 20, 2020
Merged

Conversation

orsinium
Copy link
Contributor

@orsinium orsinium commented Oct 5, 2020

If the path to config explicitly specified and it has .yaml (or .yml) extension, use YAML format instead of JSON.

Motivation

JSON is human-readable but not human-editable. Beehive WebUI is old and limited. For example, I don't see a way to edit an existing chain through the WebUI. So, let's make the configuration file nice and editable.

@orsinium orsinium mentioned this pull request Oct 7, 2020
@penguwin penguwin self-requested a review October 7, 2020 15:46
@muesli
Copy link
Owner

muesli commented Oct 10, 2020

There's also a TOML config backend in the works by @penguwin. Theoretically we could support all three formats, I'm just not sure if that's a good idea. Example configs and documentation would then often diverge and confront the user with a different format, and that's probably not helpful. We decided to go with JSON originally because it was user-readable "enough", an established standard and the idea really was to get the web interface to a point where editing the config file was the exception, not the norm.

I'm still a bit torn here. Open for discussion!

@rubiojr
Copy link
Collaborator

rubiojr commented Oct 10, 2020

☝️same! my experience is that while I'd appreciate having yaml over json when I have to manually change the config what I'd really want is powerful GUIs and CLIs that do the necessary validation for me so I don't risk breaking it.
I also found that as the config file becomes larger with many bees and chains, the language isn't that relevant anymore, they become really tricky to edit without some visual aids.

@orsinium
Copy link
Contributor Author

I also like TOML much more (Luigi supports TOML because of me, bandit is almost there) but YAML (unfortunately?) works much better for BeeHive:

  1. All fields are text: Name, Class, Description, ID, Bee, and so on. Only some rare options are numbers, and the validation won't happen even for these cases because beehive accepts interface{} as options value.
  2. TOML is good for flat configs but is not so good for deeply nested arrays:
    1. It's hard to read
    2. It's hard to write
  3. More people are familiar with YAML, at least because of docker-compose. In the DevOps world, everything is configured by YAML.
  4. Starlark #341 brings starlark support, also bash scripts can come soon. It requires multiline strings, and the indentation is important. And YAML is better (and more tricky sometimes, yes) in multiline strings, supporting different scalars and ways to fix indentation. However, TOML always preserves all spaces inside of multiline strings.

Let's see an example.

For YAML, it is clear what is related to what, nesting comes naturally:

bees:
  - name: something
    options:
      - name: hello
        value: world
      - name: another
        value: one bite
  - name: something else
    options:
      - name: check
        value: |
          def main(**kwargs):
              return True

For TOML, it is strange non-intuitive [[..]] syntax where options specified as [[bees.options]] but this is not a bees.options field but a field in one of the bees:

[[bees]]
name = "something"

  [[bees.options]]
  name = "hello"
  value = "world"

  [[bees.options]]
  name = "another"
  value = "one bite"

[[bees]]
name = "something else"

  [[bees.options]]
  name = "check"
  value = """
def main(**kwargs):
    return True
"""

So, if you don't want to support both formats at once, I advise you to choose YAML. Sometimes, YAML is tricky and don't strict enough but this shouldn't be an issue with BeeHive, and TOML would introduce more issues.

Copy link
Collaborator

@penguwin penguwin left a comment

Choose a reason for hiding this comment

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

I'd also rather prefer to support only one config format at a time. Some form of soft migration would be nice, reading in "old" json configs as usual and then writing them in the new format on save.
@orsinium made some good points for using yaml as the format for beehive and I see no reason not to use it.

@muesli
Copy link
Owner

muesli commented Oct 15, 2020

Ok, some good points there, I'm almost convinced 😄

We should still only support one designated "config language", so I agree with @penguwin: how about we try to parse the old JSON configs and migrate them to YAML when we first encounter them?

@orsinium
Copy link
Contributor Author

Thank you for your reviews!

The PR introduces support for both formats at once, without breaking the existing installations. So, it's safe to take it as is. The migration logic can be done later, in a separate PR. Meanwhile, we can try how the new format works without forcing anyone to migrate :)

@muesli
Copy link
Owner

muesli commented Oct 20, 2020

Fair enough, let's give this a chance and see how it feels.

@muesli muesli merged commit d5e9413 into muesli:master Oct 20, 2020
@muesli
Copy link
Owner

muesli commented Oct 20, 2020

Thank you @orsinium, loving all the contributions!

@orsinium orsinium deleted the yaml branch October 20, 2020 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants