feat(importer): update removed rulebases in db #3894
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds functionality to properly track and mark removed rulebases during configuration imports. Previously, when a rulebase was deleted, it was only being skipped in processing without being explicitly marked as removed in the database.
Key changes:
- Added tracking of removed rulebases by collecting their UIDs when they're not found in the current configuration
- Implemented
mark_rulebases_removed()function to mark deleted rulebases in the database - Updated
remove_outdated_refs()documentation to clarify it handles references for both changed and removed rules
Comments suppressed due to low confidence (2)
roles/importer/files/importer/model_controllers/fwconfig_import_rule.py:131
- Variable num_removed_rulebases is not used.
num_removed_rulebases = self.mark_rulebases_removed(removed_rulebase_uids)
roles/importer/files/importer/model_controllers/fwconfig_import_rule.py:794
- Variable logger is not used.
logger = getFwoLogger()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| num_new_refs = self.add_new_refs(prevConfig) | ||
|
|
||
| num_deleted_rules, removed_rule_ids = self.mark_rules_removed(rule_order_diffs["deleted_rule_uids"]) | ||
| num_removed_rulebases = self.mark_rulebases_removed(removed_rulebase_uids) |
There was a problem hiding this comment.
The variable num_removed_rulebases is assigned but never used. Consider adding it to the statistics tracking (similar to how num_deleted_rules is added to RuleDeleteCount on line 140) or logging the count of removed rulebases for visibility.
| return changes, collectedRemovedRuleIds | ||
|
|
||
| def mark_rulebases_removed(self, removedRulebaseUids: list[str]) -> int: | ||
| logger = getFwoLogger() |
There was a problem hiding this comment.
The variable logger is declared but never used in this function. Consider removing it or adding appropriate logging statements (e.g., logging when rulebases are successfully marked as removed).
|
|
||
| if len(removedRulebaseUids) == 0: | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected on this line. Please remove it to maintain code cleanliness.
tpurschke
left a comment
tpurschke
left a comment
There was a problem hiding this comment.
Shouldn't we also mark all rules which belong to a removed rulebase as removed?
This is already happening as the ruleorder service detects those rules as changed. Not entirely sure yet how it works but I tested it and it worked. |
|
2 participants
So far we did not updated removed (e.g. renamed) rulebases in the db, which lead to inconsistencies.
Now rulebases found in the previous config are set to removed if they do not appear in the new normalized config.
This will not fix existing inconsistent database states with rulebases missing the removed field.
see #3892