refactor: Migrate parameter configs to YAML

[aborg-dev]

This is a refactoring PR to enable #8264. For full motivation see #8264 (comment)

This PR only changes the disk format and parsing logic but doesn't change the in-memory representation, so it should not lead to any visible behaviour changes.

The next step in this refactoring would be to investigate getting rid of serde_json::Value in favor of serde_yaml::Value or even better a specialized struct with much more restricted types to catch invalid config errors earlier.

[aborg-dev]

The build is currently fails because we use two versions of serde_yaml, one introduced in this PR: "0.9.16" and the other "0.8.24" used by paperclip and insta deps. I've tried to use 0.8.24 in this PR and it exposes two non-critical bugs in older YAML implementation that are caught by our tests.

We can either use 0.8.24 in this PR and fix the failing tests or update paperclip and insta, as both use serde_yaml "0.9.*" in their latest releases. I'm not sure what is the usual approach, so need some advice here.

[akhi3030]

Assuming it isn't a lot of work, updating paperclip and insta seems like the right way to move forward.

[aborg-dev]

I've tried to go the update-way, but ran into some problems described in #8320 .
I will continue working on the update, but given that it is blocked on external parties, for now I will implement workarounds for this code to work with older YAML version and add TODOs to remove them after version upgrade.

[nagisa]

Is there a reason we're picking the infamous YAML over something like TOML? We’d avoid a new dep too.

[aborg-dev]

Is there a reason we're picking the infamous YAML over something like TOML? We’d avoid a new dep too.

TL;DR: No strong preference between the two, but YAML seemed a better fit from human readability perspective for our usecase. Would love to hear the arguments againts and reconsider this.

Long story:
I've outlined some reasons in #8264 (comment), but to list them explicitly:

• Almost no change to existing config to turn it into YAML
• YAML has multi-line Map values which is useful for statements like

action_receipt_creation_send_sir: {
  old: "some_long_value"
  new: "some_other_long_value"
}

In TOML, in my understanding, this either needs to be on the same line or use a table marker which in turn means that all other definitions need to use table markers or come before this definition:

[action_receipt_creation_send_sir]
old = "some_long_value"
new = "some_other_long_value"

Overall, I think the plan is to keep the config relatively flat (up to 3 layers deep) and don't use any fancy YAML features (tags, references), so we hopefully avoid any portability and debugging issues. Also, we already depend on serde_yaml indirectly, so this is not as bad as introducing a new dependency.

[jakmeier]

@Akashin Looks good to me, please address the one little nitpick and afterwards this can be merged as far as I am concerned. But let's settle the debate around the format first. :)

Almost no change to existing config to turn it into YAML

That's kind of nice but the more I think about it, the more I am convinced this should not influence our decision too much. Long-term it doesn't matter how big the diff in the PR is.

Overall, I think the plan is to keep the config relatively flat (up to 3 layers deep) and don't use any fancy YAML features (tags, references), so we hopefully avoid any portability and debugging issues. Also, we already depend on serde_yaml indirectly, so this is not as bad as introducing a new dependency.

I agree with everything here. Although, if we could remove the dependency on serde_yaml entirely, that would be nice and we are moving one step further away from that goal.

I guess I have some personal hatred towards yaml as the spec is rather complex and often tools are not fully compatible as a result of it. (At least it used to be like this and your struggle with serde_yaml sort of confirms that.^^) But as long as we keep it simple, as you suggested, this should not be an issue.

@nagisa do you want to veto against YAML? I think TOML would work fine here, too. I don't have a strong opinion in either direction. Both should allow to add a weight (new and old) to each parameter without too much hassle. But I can see the arguments presented by @Akashin that it's a bit nicer in YAML.

[jakmeier]

This looks a bit ridiculous, parsing YAML text values as JSON. 😵‍💫 But definitely better to keep it like this for now, as all our tests operate on JSON. Maaaybe long-term we can think about cleaning up some more...

[aborg-dev]

Yes, I was hoping to get rid of this in the next refactoring PR, will give it a try and see how big of a change it is going to be. If that turns out too big, I'll add a TODO here.

[nagisa]

Not going to veto yaml. I had my fair share of terrible experiences with it, but mostly in the context of a project attempting to shoehorn it into an application where a DSL would have been more appropriate (ansible, github actions…) I heard horror stories about configuration files written in yaml too, but those also approaching CI definition levels of complexity. But this is neither of those cases 🤷

[aborg-dev]

Ok, given that there are no strong arguments against, let's try using YAML for now and reevaluate whether we need to switch to TOML/RON when we either:

• Run into YAML parsing/debugging issues
• Have problems evolving the config to support our needs