feat: Support automated config migration for separate mapper config

[jarhodes314]

Proposed changes

Add a migration tool for converting tedge.toml based configs to separate mapper configs.

To use the tool, run tedge-mapper migrate-config c8y (or with the cloud of your choosing).
To use the tool, run tedge config upgrade.

TODO:

• System tests for the separate mapper config functionality
• Add line to tedge connect summary showing where the mapper is configured
• Ensure there is a mechanism for changing the default mapper config location for new users
• Include the directory cleanup logic in the system tests
• Test that config can be read from tedge.toml if mapper has flows directory but not migrated config
• Ensure we are happy with tedge connect config summary showing the feature/temporarily hide it if not

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
• I ran just format as mentioned in CODING_GUIDELINES
• I used just check as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[codecov]

Codecov Report

❌ Patch coverage is 93.45679% with 53 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...dge_config/src/tedge_toml/tedge_config_location.rs	96.29%	6 Missing and 18 partials ⚠️
...ates/core/tedge/src/cli/config/commands/upgrade.rs	0.00%	13 Missing ⚠️
...common/tedge_config/src/tedge_toml/tedge_config.rs	80.39%	9 Missing and 1 partial ⚠️
crates/core/tedge/src/cli/log.rs	0.00%	3 Missing ⚠️
crates/common/tedge_config/src/lib.rs	91.66%	0 Missing and 1 partial ⚠️
crates/core/tedge/src/cli/config/cli.rs	0.00%	1 Missing ⚠️
crates/core/tedge/src/cli/connect/command.rs	90.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
795	0	4	795	100	2h27m13.415149999s

[didier-wenzek]

tedge-mapper migrate-config is nicely working, including for cloud profile.

It could be improved a bit to be more explicit when no config is to be migrated (because not set or already migrated).

[didier-wenzek]

Why use a hidden command for this migration tool?

[jarhodes314]

At the moment, I didn't want to expose the details for undocumented features, particularly one where the user has nothing obvious to gain by using the feature

[jarhodes314]

Particularly with @albinsuresh's point about unifying directory structure between this and the mapper flow definitions in #3892 (comment), there's a chance we may need to slightly revise the structure, which is a good reason for people not to adopt this feature until we are happy it's stable.

[didier-wenzek]

The code is clear and the tests comprehensive. There are still some open UX questions, though.

I don't think we need to "Ensure there is a mechanism for changing the default mapper config location for new users" in this PR. We can make the new mapper schema the default, when the feature and the documentation will be complete.

I like that tedge connect displays the path to the mapper configuration file. This is not a problem to have a constant path when the config has not been migrated. This will even be handy to support users during the transition phase.

I noticed that now tedge config set remove unknown properties, while this was not the case before #3889. I'm okay with that, as I don't think it was a documented behavior, but it would be good to consider restoring the former behavior.

[jarhodes314]

I noticed that now tedge config set remove unknown properties, while this was not the case before #3889. I'm okay with that, as I don't think it was a documented behavior, but it would be good to consider restoring the former behavior.

I believe this is the existing behaviour for tedge.toml even though it is new for the mapper toml files. Since pretty much no users edit these files directly, I don't think this matters.

[didier-wenzek]

From my point of view, this PR is ready to be merged, with one point still to be discussed as highlighted here #3902 (comment): do we have to revise the mapper config directory structure so we can add later other mapper related configs (flows, bridged topics)?

[albinsuresh]

Just some remarks on the UX.

1. Why not restart the corresponding service after the config file migration? If we have intentionally left it to the user, thenit'd be good to give a hit about that to the user in the output. I prefer implicit restart as leaving the config migrated without the restart feels risky, as the user might forget about it and it'll unknowingly take effect with a future restart. Doing the restart inline gives us the opportunity to revert the changes as well, if it fails.
2. Why expose the migration step via the tedge-mapper cli and not tedge config? That feels like an impl detail leak that only the mapper configs are migrated. In future if we decide to move more configs around, having a singular "migration interface" would have been nice. I'm thinking something like tedge config upgrade that will detect the existing config version and migrate it to the target version that the command was invoked with. Even at the code level, the migrate-config subcommand felt misplaced.
3. The current API forces the user to do the migration one-by-one for each cloud, presumably to make the transition less error prone as the user would be able to validate things before proceeding to the next migration. But for all cloud profiles of a given cloud, the migration is done in one-shot and here we are inconsistent with the other behaviour. I'd be i favour of migrating all at once.
4. Wouldn't it be safer to keep a backup of the old config at /etc/tedge/tedge.toml.old so that the user can revert back to it just in case something fails after the migration?

[jarhodes314]

1. Why not restart the corresponding service after the config file migration? If we have intentionally left it to the user, thenit'd be good to give a hit about that to the user in the output. I prefer implicit restart as leaving the config migrated without the restart feels risky, as the user might forget about it and it'll unknowingly take effect with a future restart. Doing the restart inline gives us the opportunity to revert the changes as well, if it fails.

We could potentially do this, however it's also the case that this migration shouldn't change anything. If we did support relative paths in config files in the future then this could be a problematic assumption, but I don't see much

2. Why expose the migration step via the tedge-mapper cli and not tedge config? That feels like an impl detail leak that only the mapper configs are migrated. In future if we decide to move more configs around, having a singular "migration interface" would have been nice. I'm thinking something like tedge config upgrade that will detect the existing config version and migrate it to the target version that the command was invoked with. Even at the code level, the migrate-config subcommand felt misplaced.

This is a good point, particularly since we only install tab-completions for tedge and not tedge-mapper so if it's under tedge it will be easier to use.

3. The current API forces the user to do the migration one-by-one for each cloud, presumably to make the transition less error prone as the user would be able to validate things before proceeding to the next migration. But for all cloud profiles of a given cloud, the migration is done in one-shot and here we are inconsistent with the other behaviour. I'd be i favour of migrating all at once.

Particularly given the new default behaviour is currently set to only create separate mapper configs if no cloud configs exist in tedge.toml, this makes sense. I think it adds to the whole point of encapsulating the details of what the config migration/update is.

4. Wouldn't it be safer to keep a backup of the old config at /etc/tedge/tedge.toml.old so that the user can revert back to it just in case something fails after the migration?

Maybe. If we did this, I would like there to be a clear process through which we remove this file automatically, since leaving it around will mostly likely cause confusion. This is particularly true since we generally encapsulate all of the configuration files so the users generally never need touch them.

[albinsuresh]

4. Wouldn't it be safer to keep a backup of the old config at /etc/tedge/tedge.toml.old so that the user can revert back to it just in case something fails after the migration?

Maybe. If we did this, I would like there to be a clear process through which we remove this file automatically, since leaving it around will mostly likely cause confusion. This is particularly true since we generally encapsulate all of the configuration files so the users generally never need touch them.

If we do the service restarts as part of the "migration" then we can cleanly remove the backup after the service restarts and connection tests are deemed successful.