Skip to content

Conversation

g-linville
Copy link
Member

I've tested all of these with curl and they work properly.

Comment on lines +62 to +64
mux.HandleFunc("POST /credentials", s.listCredentials)
mux.HandleFunc("POST /credentials/create", s.createCredential)
mux.HandleFunc("POST /credentials/reveal", s.revealCredential)
Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if these routes should change at all or if this is fine. Feels kinda weird to have it this way, but it's also abnormal to do a request body with a GET, so...I just did this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The alternative is to use path and query parameters (e.g. /credentials/reveal/name?context=my-context or /credentials/delete/name?context=my-context (and make this DELETE)).

I leave the ultimate decision up to you. We certainly aren't building a REST API. This is more of RPC stuff, which I think is almost always POST.

I'll leave a final review after your response.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I think I will leave it as it is then. All POST is what works best with the way the Go SDK is set up, at least.

Comment on lines +20 to +21
// TODO - are we sure we want to always use runtimes.Default here?
store, err := credentials.NewStore(cfg, runtimes.Default(s.gptscriptOpts.Cache.CacheDir), ctx, s.gptscriptOpts.Cache.CacheDir)
Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, runtimes.Default is the only RuntimeManager in GPTScript, as far as I am aware. I would initialize this the normal way through s.gptscriptOpts, but the Runner opts there are nil, so I just did this instead.

Signed-off-by: Grant Linville <[email protected]>
@g-linville g-linville merged commit 90e7868 into gptscript-ai:main Sep 13, 2024
10 checks passed
@g-linville g-linville deleted the sdkserver-credentials branch September 13, 2024 16:57
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