Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(platform): Support manually setting up webhooks #8750

Closed
wants to merge 87 commits into from

Conversation

ntindle
Copy link
Member

@ntindle ntindle commented Nov 24, 2024

The webhooks system as is works really well for full blown enterprise webhooks managed via a UI. It does not work for more "chill guy" webhook tools that just send notifications sometimes.

Changes 🏗️

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • ...
Example test plan
  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

For configuration changes:

  • .env.example is updated or already compatible with my changes
  • docker-compose.yml is updated or already compatible with my changes
  • I have included a list of my configuration changes in the PR description (under Changes)
Examples of configuration changes
  • Changing ports
  • Adding new services that need to communicate with each other
  • Secrets or environment variable changes
  • New or infrastructure changes such as databases

Pwuts and others added 30 commits October 17, 2024 00:11
…inier/open-1961-implement-github-on-pull-request-block
…inier/open-1961-implement-github-on-pull-request-block
Pwuts and others added 8 commits November 21, 2024 19:00
Resolves #8357

- Add user flow to explicitly confirm deleting credentials linked to in-use webhooks
- Add logic to deregister, remove, and unlink webhooks before deleting parent credentials
- backend: Add `NeedConfirmation` exception
- frontend: Add `AlertDialog` UI component
Resolves #8738

- Disable webhook blocks if `PLATFORM_BASE_URL` is not set
- Throw `MissingConfigError` when trying to set up a node-webhook-link if `PLATFORM_BASE_URL` is not set
- Add `MissingConfigError`
- Add field validator to `Config.platform_base_url` and `Config.frontend_base_url`
Comment on lines +38 to +43
credentials: CredentialsMetaInput[Literal["compass"], Literal["api_key"]] = (
CredentialsField(
provider="compass",
supported_credential_types={"api_key"},
)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have any real concept of credentials that I need here. If we generate a secret on the platform side, that would be useful so we can ensure its a trusted source who sent the hook. Maybe we should store that in the credential?

Copy link
Member

Choose a reason for hiding this comment

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

For that we already have the Webhook.secret. This mechanism doesn't need credentials, so we should just omit the credentials field.

Copy link
Member

@Pwuts Pwuts Nov 25, 2024

Choose a reason for hiding this comment

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

We'll have to amend this check in on_node_activate to only enforce credentials if there is an actual credentials field on block.input_schema:

has_everything_for_webhook = (
resource is not None
and CREDENTIALS_FIELD_NAME in node.input_default
and event_filter_input_name in node.input_default
and any(is_on for is_on in node.input_default[event_filter_input_name].values())
)
if has_everything_for_webhook and resource:
logger.debug(f"Node #{node} has everything for a webhook!")
if not credentials:
credentials_meta = node.input_default[CREDENTIALS_FIELD_NAME]
raise ValueError(
f"Cannot set up webhook for node #{node.id}: "
f"credentials #{credentials_meta['id']} not available"
)

This is also one of the two places where the presence of an event filter is enforced.

Copy link
Member

@Pwuts Pwuts Nov 25, 2024

Choose a reason for hiding this comment

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

Basically the only thing it should do for your case is create a Webhook and store it in the DB. The easy way is to just skip parts of the process and then display the webhook ingress URL and webhook secret on the block in the UI. The neat way would be to have a picker for the webhooks for a given provider, on blocks that can't auto-create their webhook (like GitHub).

@@ -1,5 +1,8 @@
from typing import TYPE_CHECKING

from backend.integrations.webhooks.simple_webhook_manager import CompassWebhookManager, SimpleWebhooksManager
from pyparsing import C
Copy link
Member Author

Choose a reason for hiding this comment

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

auto imports strike again

Copy link
Member

Choose a reason for hiding this comment

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

what's next, the rest of the alphabet? :+)

Comment on lines +24 to +52
class SimpleWebhooksManager(BaseWebhooksManager):
PROVIDER_NAME = "simple_hook_manager"

DEFAULT_HEADERS = {"Accept": "application/json"}

async def _register_webhook(
self,
credentials: Credentials,
webhook_type: CompassWebhookType,
resource: str,
events: list[str],
ingress_url: str,
secret: str,
) -> tuple[str, dict]:

webhook_id = uuid.uuid4()
config = {}

return str(webhook_id), config

async def _deregister_webhook(
self, webhook: integrations.Webhook, credentials: Credentials
) -> None:
if webhook.credentials_id != credentials.id:
raise ValueError(
f"Webhook #{webhook.id} does not belong to credentials {credentials.id}"
)
# If we reach here, the webhook was successfully deleted or didn't exist

Copy link
Member Author

Choose a reason for hiding this comment

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

all i need is a url and data shape to make my example work. Not sure what is needed for that but this feels overkill, and somehow underkill

@ntindle
Copy link
Member Author

ntindle commented Nov 24, 2024

Example of hte hooks I'm getting (ignore the get requests, that's me) https://play.svix.com/view/e_UHwNM5156Yqkb3v5LnoJTE8I0EM/2pHjEII1gqLCbg40krh0XlKvb1W

@ntindle ntindle changed the title feat: wip eod commit ref: webhooks simplification Nov 24, 2024
@@ -53,6 +53,7 @@ class BlockCategory(Enum):
COMMUNICATION = "Block that interacts with communication platforms."
DEVELOPER_TOOLS = "Developer tools such as GitHub blocks."
DATA = "Block that interacts with structured data."
HARDWARE = "Block that interacts with hardware."
Copy link
Member

Choose a reason for hiding this comment

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

ROBOTICS?

Hardware is very very broad.

Comment on lines +39 to +42
webhook_id = uuid.uuid4()
config = {}

return str(webhook_id), config
Copy link
Member

@Pwuts Pwuts Nov 25, 2024

Choose a reason for hiding this comment

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

We should avoid generating and storing fake data like this. If we can't programmatically register a webhook, _register_webhook either shouldn't be called or should return empty output. The Webhook.provider_webhook_id will simply be empty, because we don't and can't know it.

Base automatically changed from reinier/open-1961-implement-github-on-pull-request-block to dev November 25, 2024 17:42
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Nov 25, 2024
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Nov 25, 2024
Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit 23ab904
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/6744bf954a6dbc00086d022c
😎 Deploy Preview https://deploy-preview-8750--auto-gpt-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Pwuts Pwuts changed the title ref: webhooks simplification feat(platform): Support manually setting up webhooks Nov 25, 2024
@ntindle
Copy link
Member Author

ntindle commented Nov 26, 2024

This does not resolve #8748. I was narrowly targeting a time window of free time I have near Thanksgiving. That ship sailed unfortunately so closing PR

@ntindle ntindle closed this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants