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

feat(schema validation): implement backup restore #13048

Merged
merged 2 commits into from
May 17, 2024

Conversation

thalesmg
Copy link
Contributor

@thalesmg thalesmg commented May 14, 2024

Fixes https://emqx.atlassian.net/browse/EMQX-12346

Release version: e5.7

Summary

From discussions with Product, when importing a schema validation backup:

  • Existing validations (identified by `name') are left untouched.
  • No validations are removed.
  • New validations are appended to the existing list.
  • Existing validations are not reordered.

PR Checklist

Please convert it to a draft if any of the following conditions are not met. Reviewers may skip over until all the items are checked:

  • Added tests for the changes
  • Added property-based tests for code which performs user input validation
  • Changed lines covered in coverage report
  • Change log has been added to changes/(ce|ee)/(feat|perf|fix|breaking)-<PR-id>.en.md files
  • For internal contributor: there is a jira ticket to track this change
  • Created PR to emqx-docs if documentation update is required, or link to a follow-up jira ticket
  • Schema changes are backward compatible

Checklist for CI (.github/workflows) changes

  • If changed package build workflow, pass this action (manual trigger)
  • Change log has been added to changes/ dir for user-facing artifacts update

@thalesmg thalesmg marked this pull request as ready for review May 14, 2024 19:37
@thalesmg thalesmg requested a review from a team as a code owner May 14, 2024 19:37
Copy link
Member

@zmstone zmstone left a comment

Choose a reason for hiding this comment

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

i wonder if there are some inconsistent behaviour for config import comparing to config load API (which has a mode option supporting replace or merge)

Result = emqx_utils:foldl_while(
fun
({validation, RawValidation}, Acc) ->
case insert(RawValidation) of
Copy link
Member

Choose a reason for hiding this comment

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

this generates small config changes,
would it make sense to merge all configs and reload all at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@zmstone
Copy link
Member

zmstone commented May 14, 2024

i wonder if there are some inconsistent behaviour for config import comparing to config load API (which has a mode option supporting replace or merge)

There is maybe a missing support for config load API (and CLI) ?

@thalesmg thalesmg requested a review from zmstone May 15, 2024 20:08
ok;
post_config_update([?CONF_ROOT], {merge, Input}, ResultingConfig, Old, _AppEnvs) ->
#{validations := ResultingValidations} = ResultingConfig,
#{new_validations := NewValidations0} = prepare_config_merge(Input, Old),
Copy link
Member

Choose a reason for hiding this comment

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

The Old map of prepare_config_merge/2 has atom as key in pre_config_update, but in post_config_update it is binary. This was not considered in the implementation.
By the way, why is there a need to do merge again in post after it has been done in pre_config_update?
It seems to be some kind of conflict. Maybe, we could handle nothing in pre_config_update and deal with it directly in post_config_update.

Copy link
Contributor Author

@thalesmg thalesmg May 16, 2024

Choose a reason for hiding this comment

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

The Old map of prepare_config_merge/2 has atom as key in pre_config_update, but in post_config_update it is binary. This was not considered in the implementation.

Good catch.

By the way, why is there a need to do merge again in post after it has been done in pre_config_update?
It seems to be some kind of conflict. Maybe, we could handle nothing in pre_config_update and deal with it directly in post_config_update.

Indeed, there's no work needed in pre_config_update. Thanks! 🍻

Actually, we do need to repeat the work: when "merging", we only add new validations.

Copy link
Member

@zhongwencool zhongwencool left a comment

Choose a reason for hiding this comment

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

LGTM, I merge this first, Let QA team help to verify this.

@zhongwencool zhongwencool merged commit 1ac7aa1 into emqx:release-57 May 17, 2024
168 checks passed
@emqxqa
Copy link

emqxqa commented May 17, 2024

TestExecution

@thalesmg thalesmg deleted the sv-backup-r57-20240514 branch May 17, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants