-
Notifications
You must be signed in to change notification settings - Fork 43
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
Dual write rule instances to new and old tables #3486
Conversation
763b0b4
to
dbca1ab
Compare
@@ -28,7 +28,7 @@ INSERT INTO entity_profiles ( | |||
$1, | |||
$2, | |||
sqlc.arg(contextual_rules)::jsonb, | |||
FALSE | |||
TRUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, any new or updated profiles will have rules in the new rule instance table, and thus can be considered migrated.
internal/profiles/service.go
Outdated
RuleTypeID: entityRuleTuple.RuleID, | ||
Name: rule.Name, | ||
EntityType: entityType, | ||
Def: pqtype.NullRawMessage{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good, the only question I have is how would we later instantiate the rule params and defs - right now the code seems to presume that they are non-nil:
result, err = r.ingester.Ingest(ctx, inf.Entity, params.GetRule().Params.AsMap())
so we'll have to check that we don't instantiate them as nil
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one comment but overall looks good.
Relates to #3485 Any time a profile is created or updated, write the rule instances to both the new and old rule instance tables. Mark the entries in the old tables as migrated so that we skip over them when we run the migration process.
|
||
BEGIN; | ||
|
||
ALTER TABLE rule_instances ALTER COLUMN def DROP NOT NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we haven't written anything to the columns, this is safe to do.
|
||
id, err := qtx.UpsertRuleInstance(ctx, newInstance) | ||
Def: def, | ||
Params: params, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that if there's no params defined then json.Marshall returns {}
? I wasn't sure about that since the previous code had an explicit params != nil
check. But I guess we'd want to either store {}
or make sure to on retrieval convert whatever value is stored to {}
.
Relates to #3485
Any time a profile is created or updated, write the rule instances to both the new and old rule instance tables. Mark the entries in the old tables as migrated so that we skip over them when we run the migration process.
Summary
Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
Fixes #(related issue)
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: