-
Notifications
You must be signed in to change notification settings - Fork 58
feat: project templates and shareable links #4154
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
Conversation
3662209
to
4501198
Compare
pkg/api/handlers/projectmcp.go
Outdated
@@ -56,7 +56,7 @@ func convertProjectMCPServer(projectServer *v1.ProjectMCPServer, mcpServer *v1.M | |||
} | |||
pmcp.Alias = mcpServer.Spec.Alias | |||
|
|||
if cred != nil && mcpServer.Spec.SharedWithinMCPCatalogName == "" { | |||
if mcpServer.Spec.SharedWithinMCPCatalogName == "" { |
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.
Note: I removed the cred != nil
check because it was preventing the fields below from being set for "copied" mcp servers
fc72494
to
fcefcf2
Compare
fcefcf2
to
8fe8595
Compare
pkg/api/handlers/mcp.go
Outdated
@@ -210,6 +210,10 @@ func (m *MCPHandler) ListServer(req api.Context) error { | |||
items := make([]types.MCPServer, 0, len(servers.Items)) | |||
|
|||
for _, server := range servers.Items { | |||
if server.Annotations[v1.HideMCPServerAnnotation] == "true" { |
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 prevents users from seeing the snapshot project's (potentially unconfigured/unauthenticated) MCP servers in their set of enabled connectors.
These MCP servers are part of the snapshot resources that get copied by users when they hit a share link or upgrade their project.
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.
Does it make sense for these to be separate object types instead of having a tracking annotation?
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.
So something like TemplateMCPServer
for example?
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.
For reference, #4154 (comment) explains why I did this with the same objects (to reuse copying logic).
pkg/api/handlers/threads.go
Outdated
} | ||
|
||
// Populate ProjectSnapshotRevision from annotation if present | ||
if ts := thread.Annotations[v1.ProjectSnapshotRevisionAnnotation]; ts != "" { |
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.
v1.ProjectSnapshotRevisionAnnotation
is just an informational timestamp so publishers and consumers can reason about when a snapshot was last updated. It could also be a status field on the thread that gets set after an upgrade has finished.
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.
+1 for status field
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.
Done
continue | ||
} else if err != nil { | ||
// Delete all existing tasks for this thread | ||
var existing v1.WorkflowList |
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.
I changed this to copy all tasks
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.
Just noticed that there's a side-effect here that causes task threads to get nuked when you upgrade from a template. I would like to follow up with a fix for this if possible.
// - introduction message | ||
// - starter messages | ||
// - tool set | ||
// - tasks | ||
// - knowledge files | ||
// - model provider | ||
// - model | ||
// - prompt | ||
// - allowed MCP tools | ||
// - project MCP servers |
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.
I've since dropped some of these from the hash since they didn't feel like they would be destructive (e.g. allowed MCP tools).
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.
I think these all make sense.
For allowed tools for example, if I change them myself, then I won't ever be able to upgrade from the template again. What if the next upgrade of the template has the allowed tools list that I need?
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.
That's a fair point, Ill add it back.
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.
Done
@@ -1,262 +0,0 @@ | |||
<script lang="ts"> |
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.
Dropped this since it wasn't being used and simplified things a bit
@@ -1,6 +1,5 @@ | |||
<script lang="ts"> |
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.
Planning on moving this to a more aptly named component/directory down the road.
8fe8595
to
fd9e89a
Compare
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.
Overall, I think it is worth considering separate objects for the copied template objects like threads, MCP servers, project MCP servers, tasks, etc (I'm sure I missed a few). This would avoid some of the boolean flags and the fields to keep objects from showing up in the API requests.
pkg/api/handlers/threads.go
Outdated
} | ||
|
||
// Populate ProjectSnapshotRevision from annotation if present | ||
if ts := thread.Annotations[v1.ProjectSnapshotRevisionAnnotation]; ts != "" { |
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.
+1 for status field
@thedadams yeah, I had the same thought. WDYT about a single Also, FWIW, the reason it isn't this way in the PR currently is to reuse most of the same logic between:
|
742baf8
to
fdd7d7a
Compare
So I stuck with copying existing resource types since it was more straightforward. @thedadams @ivyjeong13 requesting another round of review to try to get this in |
hmm, go lint isn't failing locally. Checking it out. |
Addresses the bulk of obot-platform#4107 To be addressed in a follow-up: - Preventing users from copying projects containing MCP servers they don't have access to - Preventing copied project upgrades when the user doesn't have access to MCP servers in the latest snapshot Signed-off-by: Nick Hale <[email protected]>
fdd7d7a
to
951e4eb
Compare
had a stale local work tree. fixed |
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.
https://www.loom.com/share/0f48e1839e0c4940a94c2e3596088266
Think there's one more location that could use the dontLogErrors
for the reveal endpoint! (Loom above for reference) Also thinking the init configure form data could be turned into a common function since some form of it is being used in a few locations now. But that's more of a note to myself, not PR blocking! : ) Frontend changes LGTM
Frontend changes LGTM
Signed-off-by: Nick Hale <[email protected]>
@ivyjeong13 Thanks for the review and actually taking the time to run this!
nice catch. I'll fix that ASAP |
… logs This fixes the issue Ivy was seeing with error toasts being exposed during initial MCP server configuration after a template copy. Signed-off-by: Nick Hale <[email protected]>
@ivyjeong13 done |
Signed-off-by: Nick Hale <[email protected]>
Addresses the bulk of #4107
Loom: https://www.loom.com/share/964fd03595814f92a8072df77b95634d?sid=45918fb2-f0be-4c76-bfa9-1e7a59308a41
To be addressed in a follow-up:
don't have access to
to MCP servers in the latest snapshot