WIP: Remove per-entity tables #4279
Conversation
JAORMX
force-pushed
the
rm-entity-tables
branch
from
96331dc
to
e13fca2
Compare
|
About the failures in artifact.go - I wonder if the path of least resistance (even with a bit of manual coding that might be thrown away later) would be to substitute the database lookups like GetArtifact with wrappers around GetEntity that would for now fulfill the same contract, in case of GetArtifact still return a pb.Artifact. |
| @@ -20,4 +20,72 @@ DROP TABLE IF EXISTS repositories; | |||
| DROP TABLE IF EXISTS artifacts; | |||
| DROP TABLE IF EXISTS pull_requests; | |||
|
|
|||
| CREATE VIEW repositories AS | |||
There was a problem hiding this comment.
I'm not as familiar with views and especially replacing tables with views, so maybe this is a dumb question - but how do indexes work?
Also comparing the columns, would we miss something by not having the reminder_last_sent and updated_at columns?
There was a problem hiding this comment.
Well, we don´t really care as much about the updated_at since we care now about properties. I do wonder how reminder_last_sent would work... would that need to be then part of the central table? maybe it needs to be.
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
JAORMX
force-pushed
the
rm-entity-tables
branch
3 times, most recently
from
ad94b18
to
5dd7869
Compare
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
JAORMX
force-pushed
the
rm-entity-tables
branch
from
5dd7869
to
100f2b4
Compare
| ei.entity_type, | ||
| ere.entity_instance_id as entity_id, |
There was a problem hiding this comment.
nit: I'm sorry, I could not unsee it 😂
| ei.entity_type, | |
| ere.entity_instance_id as entity_id, | |
| ei.entity_type, | |
| ere.entity_instance_id as entity_id, |
| pr.pr_number, | ||
| a.artifact_name, | ||
| j.id as project_id, | ||
| ei.name as entity_name, |
There was a problem hiding this comment.
issue: this need to be used in the WHERE condition as well.
| FROM evaluation_statuses s | ||
| JOIN evaluation_rule_entities ere ON s.rule_entity_id = ere.id | ||
| LEFT JOIN entity_instances AS ei ON ei.id = ere.entity_instance_id |
There was a problem hiding this comment.
nit: IIRC there is an ON DELETE CASCADE clause in the foreign key of from evaluation_rule_entities, so this should be a plain JOIN.
Is there a particular reason why this is a LEFT JOIN instead? Was data migrated to the new table?
| WHERE les.profile_id = $1 AND | ||
| ( | ||
| CASE | ||
| WHEN sqlc.narg(entity_type)::entities = 'repository' AND ere.repository_id = sqlc.narg(entity_id)::UUID THEN true | ||
| WHEN sqlc.narg(entity_type)::entities = 'artifact' AND ere.artifact_id = sqlc.narg(entity_id)::UUID THEN true | ||
| WHEN sqlc.narg(entity_type)::entities = 'pull_request' AND ere.pull_request_id = sqlc.narg(entity_id)::UUID THEN true | ||
| WHEN ei.entity_id = sqlc.narg(entity_id)::UUID THEN true | ||
| WHEN sqlc.narg(entity_id)::UUID IS NULL THEN true | ||
| ELSE false | ||
| END |
There was a problem hiding this comment.
nit: very, very minor thing, but I would take the chance to rewrite this into the following
WHERE les.profile_id = $1
AND (sqlc.narg(entity_id)::UUID IS NULL OR sqlc.narg(entity_id)::UUID = ei.entity_id)
|
This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days. |
Summary
Fixes #4315
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: