Update the Catalog sync to delete the removed resources info from existing db #292

pratap0007 · 2021-07-15T12:22:03Z

This patch includes following changes

Adds a migration to add OnDelete:CASECADE constraint on existing
foreign key constraints
Update the catalog sync which compares hosted catalog resources to
existing db info of resources for same catalog while syncing catalogs and delete
resources info from the db if any resources from the hosted catalog are removed

Signed-off-by: Shiv Verma shverma@redhat.com

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Run API Unit Tests, Lint Checks, API Design, Golden Files with make api-check
Run UI Unit Tests, Lint Checks with make ui-check
Commit messages follow commit message best practices

See the contribution guide for more details.

pratap0007 · 2021-07-28T06:09:51Z

/test pull-tekton-hub-unit-tests

pratap0007 · 2021-07-30T04:59:48Z

/retest

api/pkg/service/catalog/syncer.go

piyush-garg · 2021-08-23T12:22:07Z

api/pkg/db/migration/migration.go

@@ -55,9 +56,6 @@ func Migrate(api *app.APIBase) error {
 			&model.SyncJob{},
 			&model.Scope{},
 			&model.Config{},
-			&model.Platform{},


Why are we removing these from here ?

These were added in the migration as well

why are removing platform? the other 2 will be autocreated how platform will get created on a new db installation if we remove from here?

here platforms is optional and we can keep or remove it,if we remove platforms from here it will get created by the automigration on a new db installation

okay but I didn't understood why this is optional? 🤔 on new db creation initschema

hub/api/pkg/db/migration/migration.go

Line 47 in 3c1ec7d

migration.InitSchema(func(db *gorm.DB) error {

is run right?
and if platforms is not added there still it will get created

So platform table is getting created because of this migration

hub/api/pkg/db/migration/202106281500_create_platform_tables.go

Line 24 in cc84304

func createPlatformTables(log *log.Logger) *gormigrate.Migration {

sm43 · 2021-09-01T11:08:53Z

api/pkg/service/catalog/syncer.go

+}
+
+// Delete the tags which doesn't belongs to any other existing resources
+func (s *syncer) deleteTag(txn *gorm.DB, log *zap.SugaredLogger, tags []model.ResourceTag) {


do we need to do this as part of this api ? 🤔 what if we have a clean up api which would remove things like this which are no longer required

I think yes , if we want to have a clean up api then how we would identify which things are no longer required ?

PuneetPunamiya · 2021-09-08T11:09:33Z

api/pkg/db/migration/202107221500_add_ondelete_constraints.go

+		ID: "202107221500_add_ondelete_constraints",
+		Migrate: func(db *gorm.DB) error {
+			txn := db.Begin()
+


Can you create a function like this https://github.com/tektoncd/hub/blob/main/api/pkg/db/migration/202107291608_refresh_all_tables.go#L36
and handle the error in a different function instead of writing txn.Rollback multiple times

PuneetPunamiya · 2021-09-13T12:54:26Z

api/pkg/db/migration/202107221500_add_ondelete_constraints.go

+		return err
+	}
+
+	if err := txn.Exec("ALTER TABLE resource_categories ADD CONSTRAINT fk_resource_categories_resource  FOREIGN KEY (resource_id) REFERENCES resources(id) ON DELETE CASCADE;").Error; err != nil {


Not mandatory just a thought can't we use update query from gorm instead of writing the whole query, else the patch looks fine to me

For example
Drop Constraint
update column from table by adding foreign key constraints with on Delete cascade
For ref : https://gorm.io/docs/update.html#Update-single-column

No, we can't do with gorm and with gorm CreateConstraint does't create

pratap0007 · 2021-09-15T14:02:31Z

/test pull-tekton-hub-unit-tests

PuneetPunamiya

Platform linux/amd64 is getting deleted which is still there in the other resources. Could you please take a look at it once

/hold

PuneetPunamiya · 2021-10-13T05:08:19Z

Thanks 🙂

/approve
/meow

/hold cancel

tekton-robot · 2021-10-13T05:08:20Z

@PuneetPunamiya:

In response to this:

Thanks 🙂

/approve
/meow

/hold cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2021-10-13T05:08:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PuneetPunamiya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [PuneetPunamiya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vinamra28

also at the time of migration from 1.4 to latest migration fails. Can you please look into this?
/hold

vinamra28 · 2021-10-14T04:43:23Z

api/pkg/db/migration/202107221500_add_ondelete_constraints.go

+			err := addOnDelete(txn, log)
+			if err != nil {
+				txn.Rollback()
+				log.Error(err)


nit: I guess we can avoid logging this error since we are already logging the errors in addOnDelete function

…sting db This patch includes following changes - Adds a migration to add `OnDelete:CASECADE` constraint on existing foreign key constraints - Update the catalog sync which compares hosted catalog resources to existing db info of resources for same catalog while syncing catalogs and delete resources info from the db if any resources from the hosted catalog are removed Signed-off-by: Shiv Verma <shverma@redhat.com>

pratap0007 · 2021-10-14T11:42:08Z

@vinamra28 done, could you please take a look once more

vinamra28 · 2021-10-21T11:25:29Z

/hold cancel
/lgtm

tekton-robot requested review from PuneetPunamiya and sm43 July 15, 2021 12:22

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2021

pratap0007 force-pushed the delete-removed-resource branch from 6c74cac to b29c628 Compare July 19, 2021 06:02

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2021

pratap0007 force-pushed the delete-removed-resource branch from b29c628 to 47be2b9 Compare July 22, 2021 10:18

tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 22, 2021

pratap0007 force-pushed the delete-removed-resource branch from 47be2b9 to b90c97e Compare July 22, 2021 10:21

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2021

pratap0007 force-pushed the delete-removed-resource branch 3 times, most recently from 8ef9a39 to 5cca636 Compare July 28, 2021 05:08

pratap0007 force-pushed the delete-removed-resource branch from 5cca636 to d112a15 Compare July 29, 2021 13:39

pratap0007 force-pushed the delete-removed-resource branch from d112a15 to a98edc8 Compare July 30, 2021 12:21

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2021

pratap0007 force-pushed the delete-removed-resource branch from a98edc8 to 1674c51 Compare August 16, 2021 04:58

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2021

sm43 reviewed Aug 17, 2021

View reviewed changes

api/pkg/service/catalog/syncer.go Show resolved Hide resolved

pratap0007 force-pushed the delete-removed-resource branch from 1674c51 to 63a4fa8 Compare August 20, 2021 13:59

piyush-garg reviewed Aug 23, 2021

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2021

pratap0007 force-pushed the delete-removed-resource branch from 63a4fa8 to 56a2b98 Compare September 1, 2021 09:20

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2021

pratap0007 force-pushed the delete-removed-resource branch from 56a2b98 to 3c1ec7d Compare September 1, 2021 09:22

sm43 reviewed Sep 1, 2021

View reviewed changes

PuneetPunamiya reviewed Sep 8, 2021

View reviewed changes

pratap0007 force-pushed the delete-removed-resource branch from 3c1ec7d to f8f3086 Compare September 9, 2021 09:04

PuneetPunamiya reviewed Sep 13, 2021

View reviewed changes

pratap0007 force-pushed the delete-removed-resource branch from f8f3086 to b2cfc4f Compare September 15, 2021 13:46

PuneetPunamiya reviewed Oct 12, 2021

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2021

pratap0007 force-pushed the delete-removed-resource branch from b2cfc4f to a0a060e Compare October 12, 2021 12:28

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2021

vinamra28 requested changes Oct 14, 2021

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2021

pratap0007 force-pushed the delete-removed-resource branch from a0a060e to 81e16b4 Compare October 14, 2021 11:16

tekton-robot assigned vinamra28 Oct 21, 2021

tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 21, 2021

tekton-robot merged commit efdc199 into tektoncd:main Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the Catalog sync to delete the removed resources info from existing db #292

Update the Catalog sync to delete the removed resources info from existing db #292

pratap0007 commented Jul 15, 2021 •

edited

Loading

pratap0007 commented Jul 28, 2021

pratap0007 commented Jul 30, 2021

piyush-garg Aug 23, 2021

pratap0007 Aug 30, 2021

sm43 Sep 1, 2021

pratap0007 Sep 1, 2021

sm43 Sep 1, 2021

pratap0007 Sep 1, 2021

PuneetPunamiya Oct 12, 2021

sm43 Sep 1, 2021

pratap0007 Sep 1, 2021

PuneetPunamiya Sep 8, 2021

PuneetPunamiya Sep 13, 2021

PuneetPunamiya Sep 13, 2021

pratap0007 Sep 14, 2021

pratap0007 commented Sep 15, 2021

PuneetPunamiya left a comment

PuneetPunamiya commented Oct 13, 2021

tekton-robot commented Oct 13, 2021

tekton-robot commented Oct 13, 2021

vinamra28 left a comment

vinamra28 Oct 14, 2021

pratap0007 commented Oct 14, 2021

vinamra28 commented Oct 21, 2021

Update the Catalog sync to delete the removed resources info from existing db #292

Update the Catalog sync to delete the removed resources info from existing db #292

Conversation

pratap0007 commented Jul 15, 2021 • edited Loading

Changes

Submitter Checklist

pratap0007 commented Jul 28, 2021

pratap0007 commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratap0007 commented Sep 15, 2021

PuneetPunamiya left a comment

Choose a reason for hiding this comment

PuneetPunamiya commented Oct 13, 2021

tekton-robot commented Oct 13, 2021

tekton-robot commented Oct 13, 2021

vinamra28 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratap0007 commented Oct 14, 2021

vinamra28 commented Oct 21, 2021

pratap0007 commented Jul 15, 2021 •

edited

Loading