Conversation
8e3e744 to
abe08d6
Compare
|
Heya. Thanks for working on a fix. Easy things first: DCO needs to be adjusted on the commits. Harder things: there might be other places where a data source is referenced by name only -- like https://github.com/open-policy-agent/opa-control-plane/blob/main/pkg/service/worker.go#L174-L183. Can you add some sort of e2e test so we know that it'll actually work? 💦 Thanks again 🎈 |
| fmt.Sprintf("%03d_fix_datasources_primary_key_rename.up.sql", offset): "ALTER TABLE sources_datasources RENAME TO " + oldName, | ||
| fmt.Sprintf("%03d_fix_datasources_primary_key_create.up.sql", offset+1): strings.TrimRight(newTbl.SQL(kind), ";"), | ||
| fmt.Sprintf("%03d_fix_datasources_primary_key_copy.up.sql", offset+2): fmt.Sprintf(`INSERT INTO sources_datasources (%[1]s) SELECT %[1]s FROM %[2]s`, cols, oldName), | ||
| fmt.Sprintf("%03d_fix_datasources_primary_key_drop.up.sql", offset+3): "DROP TABLE " + oldName, |
There was a problem hiding this comment.
I think we have one txn per file -- can we put this into a single transaction to avoid inconsistencies on failure? 🤔
There was a problem hiding this comment.
Thanks for the review!
• DCO: fixed
• worker.go : Tried to address this by adding a datasourcePath field to sourceSynchronizer metadata is now nested by name -> path when names collide
• E2E test: added TestBundleWorkerExecute_DuplicateDatasourceNamesDisambiguatedByPath in worker_test.go
Let me know if this properly addresses your comments or something else is required.
abe08d6 to
795ac6b
Compare
Signed-off-by: Manuel Feyrer <manuel.feyrer@zeiss.com>
795ac6b to
c4e9c3d
Compare
2 participants
Fix: data sources with the same
namebut differentpathsilently overwrite each otherFixes #416
Changes
sources_datasourcesprimary key to(source_id, name, path)via new migration (recreate table across sqlite/postgres/mysql/cockroachdb).UpsertSource's conflict key to(source_id, name, path).Datasources.Equalto key by(Name, Path)instead ofNamealone.Testing
go test ./...(sqlite, postgres, mysql, cockroachdb via testcontainers) — all pass.