Skip to content

[feat-5517]: Add Delete API for Scope configs#5558

Merged
hezyin merged 7 commits into
apache:mainfrom
merico-ai:issues/5517
Jun 26, 2023
Merged

[feat-5517]: Add Delete API for Scope configs#5558
hezyin merged 7 commits into
apache:mainfrom
merico-ai:issues/5517

Conversation

@keon94
Copy link
Copy Markdown
Contributor

@keon94 keon94 commented Jun 23, 2023

Summary

What does this PR do?

Does this close any open issues?

Closes #5517

Screenshots

Include any relevant screenshots here.

Other Information

New API added for all plugins supporting scope-configs:
DELETE /plugins/{plugin-name}/connections/{connection-id}/scope-configs/{config-scope-id} cc FYI @mintsweet (For UI)

@keon94 keon94 force-pushed the issues/5517 branch 4 times, most recently from 5345af0 to 9eeb5b6 Compare June 24, 2023 00:37
@keon94 keon94 marked this pull request as ready for review June 24, 2023 00:51
@keon94 keon94 self-assigned this Jun 24, 2023
@keon94 keon94 added this to the v0.18.0 milestone Jun 24, 2023
Comment thread backend/helpers/pluginhelper/api/connection_helper.go
if scopeModel == nil {
return nil
}
return t.db.Exec(fmt.Sprintf("UPDATE %s SET scope_config_id = NULL WHERE scope_config_id = %d", scopeModel.TableName(), scopeConfigId))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can pass the scopeModel to the db.Delete in conjunction with dal.Where clause in this case. It is preferable to use db.Delete since we can add hook to the gorm when we need to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is with Dynamic structs that we use for Python. Gorm doesn't play nicely with them, especially with update operations.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the typo, it should be db.Update.
Alright then, let's add a comment to it to avoid mis-edit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that there's a dal.UpdateColumn method that also get the job done (without writing raw sql). I've updated the code to use that.

Comment thread backend/helpers/pluginhelper/api/scope_generic_helper.go
Copy link
Copy Markdown
Contributor

@hezyin hezyin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hezyin hezyin merged commit 990c19d into apache:main Jun 26, 2023
@keon94 keon94 deleted the issues/5517 branch June 26, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][BE+UI] Delete ScopeConfig support

3 participants