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

[fix] function execution events accept interfaces not strings #1357

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ProjectBarks
Copy link

@ProjectBarks ProjectBarks commented Dec 4, 2024

API changes [BREAKING]

In the initial implementation of this change, we assumed that the input types would always be map[string]string. However, Slack can send inputs with various types, not just strings. This update modifies the mapping to use an interface{} to handle these different input types. Additionally, the test has been updated to include an example of a real payload received from Slack to ensure proper handling of these cases.

Example
{
  "event": {
    "type": "function_executed",
    "function": {
      "id": "Fn07TRTH0EKX",
      "callback_id": "d70234",
      "title": "Test",
      "description": "Test.",
      "type": "app",
      "input_parameters": [
        {
          "type": "slack#/types/message_context",
          "name": "message_context",
          "description": "Test",
          "title": "Mesage Context",
          "is_required": true
        }
      ]
    },
    "inputs": {
      "message_context": {
        "channel_id": "C07H1ER7U66",
        "message_ts": "1733331835.871019"
      },
    },
    "function_execution_id": "Fx08387NLCNT",
    "workflow_execution_id": "Wx084BHAGCMN",
    "event_ts": "1733331846.861810"
  }
}

@calebmckay
Copy link
Contributor

Code change seems simple enough.

I updated one of the automated tests to test a variety of values for the input, including the message_context type you provide in your example. You may want to pull the changes into your PR. b005e63

@ProjectBarks
Copy link
Author

ProjectBarks commented Dec 12, 2024

Oh I actually had cases to cover this but I guess I forgot to push. I'll incorporate b005e63 as well. Thanks!

@ProjectBarks
Copy link
Author

@calebmckay are we good to land this?

Copy link
Contributor

@calebmckay calebmckay left a comment

Choose a reason for hiding this comment

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

@ProjectBarks I can approve, but I'm not actually a maintainer - just kind of ad-hoc helping with some code changes and PRs.

@lorenzoaiello Is this good to merge?

@ProjectBarks
Copy link
Author

@parsley42 can you take a look?

@lorenzoaiello
Copy link
Contributor

@ProjectBarks I can approve, but I'm not actually a maintainer - just kind of ad-hoc helping with some code changes and PRs.

@lorenzoaiello Is this good to merge?

It’s good with me, but @nlopes has taken back over maintainership of the repo, so I’ll defer to him until I get an idea of how he wants to run things moving forward.

@nlopes
Copy link
Collaborator

nlopes commented Jan 16, 2025

Apologies for the lack of action here, I'm currently OOO but should get to this early next week 🙏

@nlopes nlopes self-requested a review January 27, 2025 22:32
@nlopes nlopes self-assigned this Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants