Skip to content

Fix/Multiple Reply in Git Provider#37

Merged
Amazing-Stardom merged 2 commits into
masterfrom
feat/github-comment-post
May 4, 2026
Merged

Fix/Multiple Reply in Git Provider#37
Amazing-Stardom merged 2 commits into
masterfrom
feat/github-comment-post

Conversation

@Amazing-Stardom
Copy link
Copy Markdown
Contributor

This PR is for solving this issue: #35

Problem

As the main problem was with the webhook. On deleting any connector, it was not deleting the webhook that was created on initial connection.

Solution

On delete, the controller API is called.

  1. Fetch all webhook details from the DB and create a delete job queue to delete the webhook in providers.
  2. Delete both the webhook and integration ID present in the DB. (which is currently happening correct implementation)

LiveReview Pre-Commit Check: ran (iter:4, coverage:64%)
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
@Amazing-Stardom
Copy link
Copy Markdown
Contributor Author

Amazing-Stardom commented May 4, 2026

Note: This is Livereview PR comments.

Severity: info

Implement Conditional Webhook Registry Updates

Overview

Introduced a SkipRegistryUpdate flag for webhook removal jobs. This flag controls updates to the webhook_registry table. Its value now depends on the specific webhook removal context.

Technical Highlights

  • internal/jobqueue/jobqueue.go: Added SkipRegistryUpdate field to webhook removal job arguments and handlers.
  • internal/api/connectors.go: Queues webhook removal jobs with SkipRegistryUpdate set to true during connector deletion.
  • internal/api/server.go: Queues webhook removal jobs with SkipRegistryUpdate set to false when disabling manual triggers.

Impact

  • Functionality: Provides granular control over webhook_registry updates during webhook removal operations.
  • Risk: Incorrect SkipRegistryUpdate usage may lead to webhook_registry data inconsistencies.

Comment thread internal/api/connectors.go
}

// Validate connector ownership
if _, err := s.validateConnectorOwnership(c, id); err != nil {
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.

Severity: critical

Crucial security check: ensures a user can only delete connectors they own.

Suggestions:

  1. Verify validateConnectorOwnership covers all necessary authorization checks.

Comment thread internal/api/connectors.go
Comment thread internal/api/connectors.go
Comment thread internal/api/connectors.go
Comment thread internal/api/connectors.go
Comment thread internal/api/server.go
@Amazing-Stardom Amazing-Stardom marked this pull request as ready for review May 4, 2026 14:53
Provider string `json:"provider"`
BaseURL string `json:"base_url"`
PAT string `json:"pat"`
SkipRegistryUpdate bool `json:"skip_registry_update"`
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.

what is this new type?

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 webhook delete queue function had a DB update dependency.

  1. Delete webhook from provider
  2. Update webhook_registry table data.

Proposed API for Deleting Git Connector Have this.

  1. Get webhook from the DB and then create job to delete it.
    if err := s.jobQueue.QueueWebhookRemovalJob(ctx, id, projectPath, provider, providerURL, patToken, true); err != nil {
  2. Delete the integration token.
    result, err := s.db.Exec(`

    As it webhook_registry is a child table, it will be also deleted.
  3. Return response to UI

So, as I don't want to update webhook_registrythe table, I will skip it.

let me know if the variable name needs update.

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.

The variable name SkipRegistryUpdate is clear and accurately reflects its purpose. It's a good choice given the context you've provided about the webhook_registry table being automatically deleted when the parent integration_tokens (connector) is removed.

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.

okay

@Amazing-Stardom Amazing-Stardom merged commit 6137226 into master May 4, 2026
16 checks passed
@LinceMathew LinceMathew deleted the feat/github-comment-post branch May 18, 2026 15:56
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.

2 participants