search

rancher / elemental-operator

The Elemental operator is responsible for managing the OS versions and maintaining a machine inventory to assist with edge or baremetal installations.
Apache License 2.0
40 stars 17 forks source link

Use YAML content for Elemental Agent config #765

Closed anmazzotti closed 2 months ago

anmazzotti commented 2 months ago

Fixes #764 Should also be backported to 1.6.x

fgiudici commented 2 months ago

Good catch! Well, seems we are lucky enough that the kuberntes-sigs yaml parser can also fix our use case: it allows to use json struct tags to serialize with yaml (see https://github.com/kubernetes-sigs/yaml ). So, if that is true, it should be as easy as using yaml.Marshal instead of json.Marshal in the getAgentConfigBytes() here: https://github.com/rancher/elemental-operator/blob/2c3f177fd253eb0c401890fabed319667aced9cf/pkg/install/install.go#L409C1-L409C52

anmazzotti commented 2 months ago

I'd like not to depend on that yaml parser if possible, as I find it a bit confusing to not have explicit yaml definitions. That's how currently works however, since that parser is used by the system-agent.

Just using .json is also supported, so I opted for that route since we already marshal it.

fgiudici commented 2 months ago

It is perfectly fine to rename the config file to .json (since the contents are already in json format indeed). Anyway, what doesn't look that great is to introduce branches in the code to manage old and new versions. I know we already do in some parts, but would be better if we could avoid it whenever possible.

anmazzotti commented 2 months ago

It is perfectly fine to rename the config file to .json (since the contents are already in json format indeed). Anyway, what doesn't look that great is to introduce branches in the code to manage old and new versions. I know we already do in some parts, but would be better if we could avoid it whenever possible.

That only applies to the elemental-support, and it wasn't even working previously anyway, so I could simply eliminate the branch there and collect .json only.

Nevermind, we would still need to collect both, since it could be that the .yaml one is still present after an OS upgrade.

So I can replace the yaml parser as you suggested, but this will apply to the entire installer, or we can use 2 different yaml parsers for this specific scope, which I also don't 100% like.

anmazzotti commented 2 months ago

@fgiudici I refactored it to actually use YAML after you pointed out conflict with the elemental-system-agent.service file.