-
Notifications
You must be signed in to change notification settings - Fork 651
Add support for running an Actions workflow #269
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
d776940
39099ec
1c6285c
e89ccf6
034621b
1b10c0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
package github | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
|
||
"github.com/github/github-mcp-server/pkg/translations" | ||
"github.com/google/go-github/v69/github" | ||
"github.com/mark3labs/mcp-go/mcp" | ||
"github.com/mark3labs/mcp-go/server" | ||
) | ||
|
||
// RunWorkflow creates a tool to run an Actions workflow | ||
func RunWorkflow(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { | ||
return mcp.NewTool("run_workflow", | ||
mcp.WithDescription(t("TOOL_RUN_WORKFLOW_DESCRIPTION", "Trigger a workflow run")), | ||
mcp.WithString("owner", | ||
mcp.Required(), | ||
mcp.Description("The account owner of the repository. The name is not case sensitive."), | ||
), | ||
mcp.WithString("repo", | ||
mcp.Required(), | ||
mcp.Description("Repository name"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No mention of case-sensitivity here. Does that imply that it is case-sensitive? Should we be explicit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches the description used by most/all of the other I was thinking it might make sense to define some constant/reusable input definitions for these common inputs like repo & owner for consistency. |
||
), | ||
mcp.WithString("workflowId", | ||
mcp.Required(), | ||
mcp.Description("The ID of the workflow. You can also pass the workflow file name as a string."), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just the workflow file name itself is acceptable? Or the relative path to the workflow file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API supports both Relative path gh api -X POST /repos/joshmgross/actions-testing/actions/workflows/hello-world.yaml/dispatches -f "ref=main" Full path (note the gh api -X POST /repos/joshmgross/actions-testing/actions/workflows/.github%2Fworkflows%2Fhello-world.yaml/dispatches -f "ref=main" I don't think func (s *ActionsService) CreateWorkflowDispatchEventByFileName(ctx context.Context, owner, repo, workflowFileName string, event CreateWorkflowDispatchEventRequest) (*Response, error) {
u := fmt.Sprintf("repos/%v/%v/actions/workflows/%v/dispatches", owner, repo, workflowFileName)
return s.createWorkflowDispatchEvent(ctx, u, &event)
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be tempted to flip this around too, and call it Does that make sense? The goal here is to make an API that LLMs comprehend and use natively, so definitely have a think on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed to |
||
), | ||
mcp.WithString("ref", | ||
mcp.Required(), | ||
mcp.Description("Git reference (branch or tag name)"), | ||
), | ||
mcp.WithObject("inputs", | ||
mcp.Description("Input keys and values configured in the workflow file."), | ||
), | ||
), | ||
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
owner, err := requiredParam[string](request, "owner") | ||
if err != nil { | ||
return mcp.NewToolResultError(err.Error()), nil | ||
} | ||
repo, err := requiredParam[string](request, "repo") | ||
if err != nil { | ||
return mcp.NewToolResultError(err.Error()), nil | ||
} | ||
workflowID, err := requiredParam[string](request, "workflowId") | ||
if err != nil { | ||
return mcp.NewToolResultError(err.Error()), nil | ||
} | ||
ref, err := requiredParam[string](request, "ref") | ||
if err != nil { | ||
return mcp.NewToolResultError(err.Error()), nil | ||
} | ||
|
||
// Get the optional inputs parameter | ||
var inputs map[string]any | ||
if inputsObj, exists := request.Params.Arguments["inputs"]; exists && inputsObj != nil { | ||
inputs, _ = inputsObj.(map[string]any) | ||
} | ||
|
||
// Convert inputs to the format expected by the GitHub API | ||
inputsMap := make(map[string]any) | ||
if inputs != nil { | ||
for k, v := range inputs { | ||
inputsMap[k] = v | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was mostly generated by Copilot, not sure if there's a better way to process an object parameter. |
||
|
||
// Create the event to dispatch | ||
event := github.CreateWorkflowDispatchEventRequest{ | ||
Ref: ref, | ||
Inputs: inputsMap, | ||
} | ||
|
||
client, err := getClient(ctx) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to get GitHub client: %w", err) | ||
} | ||
|
||
resp, err := client.Actions.CreateWorkflowDispatchEventByFileName(ctx, owner, repo, workflowID, event) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to trigger workflow: %w", err) | ||
} | ||
defer func() { _ = resp.Body.Close() }() | ||
|
||
result := map[string]any{ | ||
"success": true, | ||
"message": "Workflow triggered successfully", | ||
} | ||
|
||
r, err := json.Marshal(result) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to marshal response: %w", err) | ||
} | ||
|
||
return mcp.NewToolResultText(string(r)), nil | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
package github | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"net/http" | ||
"testing" | ||
|
||
"github.com/github/github-mcp-server/pkg/translations" | ||
"github.com/google/go-github/v69/github" | ||
"github.com/migueleliasweb/go-github-mock/src/mock" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func Test_RunWorkflow(t *testing.T) { | ||
// Verify tool definition once | ||
mockClient := github.NewClient(nil) | ||
tool, _ := RunWorkflow(stubGetClientFn(mockClient), translations.NullTranslationHelper) | ||
|
||
assert.Equal(t, "run_workflow", tool.Name) | ||
assert.NotEmpty(t, tool.Description) | ||
assert.Contains(t, tool.InputSchema.Properties, "owner") | ||
assert.Contains(t, tool.InputSchema.Properties, "repo") | ||
assert.Contains(t, tool.InputSchema.Properties, "workflowId") | ||
assert.Contains(t, tool.InputSchema.Properties, "ref") | ||
assert.Contains(t, tool.InputSchema.Properties, "inputs") | ||
assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "workflowId", "ref"}) | ||
|
||
tests := []struct { | ||
name string | ||
mockedClient *http.Client | ||
requestArgs map[string]any | ||
expectError bool | ||
expectedErrMsg string | ||
}{ | ||
{ | ||
name: "successful workflow trigger", | ||
mockedClient: mock.NewMockedHTTPClient( | ||
mock.WithRequestMatchHandler( | ||
mock.PostReposActionsWorkflowsDispatchesByOwnerByRepoByWorkflowId, | ||
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
w.WriteHeader(http.StatusNoContent) | ||
}), | ||
), | ||
), | ||
requestArgs: map[string]any{ | ||
"owner": "owner", | ||
"repo": "repo", | ||
"workflowId": "workflow_id", | ||
"ref": "main", | ||
"inputs": map[string]any{ | ||
"input1": "value1", | ||
"input2": "value2", | ||
}, | ||
}, | ||
expectError: false, | ||
}, | ||
{ | ||
name: "missing required parameter", | ||
mockedClient: mock.NewMockedHTTPClient(), | ||
requestArgs: map[string]any{ | ||
"owner": "owner", | ||
"repo": "repo", | ||
"workflowId": "main.yaml", | ||
// missing ref | ||
}, | ||
expectError: true, | ||
expectedErrMsg: "missing required parameter: ref", | ||
}, | ||
} | ||
|
||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
// Setup client with mock | ||
client := github.NewClient(tc.mockedClient) | ||
_, handler := RunWorkflow(stubGetClientFn(client), translations.NullTranslationHelper) | ||
|
||
// Create call request | ||
request := createMCPRequest(tc.requestArgs) | ||
|
||
// Call handler | ||
result, err := handler(context.Background(), request) | ||
|
||
require.NoError(t, err) | ||
require.Equal(t, tc.expectError, result.IsError) | ||
|
||
// Parse the result and get the text content if no error | ||
textContent := getTextResult(t, result) | ||
|
||
if tc.expectedErrMsg != "" { | ||
assert.Equal(t, tc.expectedErrMsg, textContent.Text) | ||
return | ||
} | ||
|
||
// Unmarshal and verify the result | ||
var response map[string]any | ||
err = json.Unmarshal([]byte(textContent.Text), &response) | ||
require.NoError(t, err) | ||
assert.Equal(t, true, response["success"]) | ||
assert.Equal(t, "Workflow triggered successfully", response["message"]) | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of Actions APIs, I suspect the single-file per API area will break down at some point.
I tried defining a separate
actions
package, but it created some circular dependencies with thisgithub
package.I'd be happy to explore that more, but I wanted to keep these changes focused on this tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate you mentioning though, it's same reason I didn't consider new packages for tools yet.
Pretty sure the helpers and server can be extracted to their own packages to avoid this.
CC @williammartin @juruen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might open an interesting debate that really resonates with me!
When I started writing Go, I came from other languages where quickly deciding to use packages was the norm. So I followed suit, and my initial reaction was to do the same. However, over time, I've come to terms with the fact that this might not be entirely idiomatic.
Take
go-github
, for example — they implement almost the entire GitHub API within a single package.So my point here is that maybe we don't need to introduce more packages right away. We might consider another alternative, such as splitting the functionality into files like
actions_foo.go
,actions_bar.go
, etc., and that might be more idiomatic.Having said that, I don't have a strong opinion on it, but I'd like us to consider the above before we make the decision.
Thanks! ❤️