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

chore: gitlab tools #314

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: gitlab tools #314

wants to merge 1 commit into from

Conversation

drpebcak
Copy link
Contributor

@drpebcak drpebcak commented Jan 6, 2025

Signed-off-by: Taylor Price <[email protected]>
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

Some comments, but I don't want to hold this PR up on them.

@@ -0,0 +1,7 @@
Name: GitLab Credential
Share Credential: github.com/gptscript-ai/credential as gitlabToken
Copy link
Member

Choose a reason for hiding this comment

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

nit: we'll probably want to pull this into the tools repo at some point

Comment on lines +257 to +263
fmt.Printf("* Issue #%d: %s\n", issue.IID, issue.Title)
fmt.Printf(" Issue ID: %d\n", issue.ID)
fmt.Printf(" Project Name: %s\n", project.Name)
fmt.Printf(" Project ID: %d\n", issue.ProjectID)
fmt.Printf(" State: %s\n", issue.State)
fmt.Printf(" Author Username: %s\n", issue.Author.Username)
fmt.Printf(" Author ID: %d\n", issue.Author.ID)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Where do we stand on returning minified JSON output vs custom plain text structures like this? In my experience returning minified JSON to the LLM yields pretty good results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure we have an official stance, but this is how we have formatted the data in a few other places.

Comment on lines +37 to +41
query, err = parseStringToMap(rawQuery)
if err != nil {
fmt.Printf("Failed to parse ISSUE_QUERY: %v\n", err)
os.Exit(1)
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: if you factor this switch out into a separate function, you can return errors and have a single fmt.Println(err); os.Exit(1); in main().

#!${GPTSCRIPT_TOOL_DIR}/bin/gptscript-go-tool approveMergeRequest

---
Name: Lookup User ID
Copy link
Member

Choose a reason for hiding this comment

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

nit: may also be nice to have a Search Users tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, but it was not part of the scope in the original issue. Lookup User ID exists only to get the user ID for calls that require a numeric ID instead of accepting a username.

Description: Takes a username and returns the ID for that user.
Share Context: GitLab Context
Credential: ./credential
Param: gitlab_username: A GitLab username.
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect this tool to get called a lot? If so, I'd consider optimizing it by making this tool a bulk operation that retrieves a list of user IDs given a set of usernames.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think it would be called very often. Most operations accept a username, but there's one or two that require the numeric ID. Those sorts of calls wouldn't typically need to get a bunch of IDs at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these should be added to the top-level .gitignore file.

Comment on lines +6 to +10
lines := strings.Split(s, "\n")
for i, line := range lines {
lines[i] = " " + line
}
return strings.Join(lines, "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is slightly more efficient?

Suggested change
lines := strings.Split(s, "\n")
for i, line := range lines {
lines[i] = " " + line
}
return strings.Join(lines, "\n")
return " " + strings.ReplaceAll(s, "\n", "\n ")


notes, _, err := client.Notes.ListIssueNotes(projectID, issueIID, &gitlab.ListIssueNotesOptions{})
if err != nil {
log.Fatalf("Failed to fetch issue notes: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just return this error and have the top-level error handling do it?

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.

3 participants