feat: add stacked credential contexts #849
Conversation
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
| @@ -64,7 +64,7 @@ type GPTScript struct { | |||
| Chdir string `usage:"Change current working directory" short:"C"` | |||
| Daemon bool `usage:"Run tool as a daemon" local:"true" hidden:"true"` | |||
| Ports string `usage:"The port range to use for ephemeral daemon ports (ex: 11000-12000)" hidden:"true"` | |||
| CredentialContext string `usage:"Context name in which to store credentials" default:"default"` | |||
There was a problem hiding this comment.
Our cmd library doesn't support default values for slices, which is why I removed the default tag here.
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
g-linville
changed the title
Signed-off-by: Grant Linville <[email protected]>
g-linville
requested review from
thedadams,
StrongMonkey,
iwilltry42 and
tylerslaton
g-linville
changed the title
pkg/cli/credential.go
Outdated
| } else if len(ctxs) == 0 { | ||
| ctxs = []string{credentials.DefaultCredentialContext} |
There was a problem hiding this comment.
There are a few places where you have added this logic. Can this logic be moved to the Complete function for the gptscript options instead?
There was a problem hiding this comment.
I think it already is in the Complete function in the gptscript/gptscript.go file. But without it in places like this, the c.root.CredentialContext still ends up being empty when the user doesn't specify anything. Not sure why that's the case. Should I be manually calling the Complete function here?
There was a problem hiding this comment.
I'm surprised this isn't working as I expect, then.
There was a problem hiding this comment.
I took a longer look at this: instead of completing the entire opts (created on line 53), we are completing individual pieces of it (lines 55 and 56).
It would be better to complete everything instead of the individual pieces, but I won't hold up merging this based on that.
There was a problem hiding this comment.
Sweet, that worked. Thanks. I should have spent more time looking at this myself, lol
| @@ -58,7 +58,7 @@ type toolOrFileRequest struct { | |||
| ChatState string `json:"chatState"` | |||
| Workspace string `json:"workspace"` | |||
| Env []string `json:"env"` | |||
| CredentialContext string `json:"credentialContext"` | |||
| CredentialContext []string `json:"credentialContext"` | |||
There was a problem hiding this comment.
Can we keep this as a string in the API to not break backwards compatibility Split in the implementation?
There was a problem hiding this comment.
Darren specifically asked me to change this in the API. My SDK changes haven't been merged yet so this isn't really a breaking change, since nothing should be using it.
There was a problem hiding this comment.
It's a breaking change because older versions of the SDK won't work with the newer versions.
If Darren specifically asked for it, then that's fine.
There was a problem hiding this comment.
ohhhh I just noticed that this is in the "toolOrFileRequest" and not in the Credentials struct. My bad. You are correct about this
Signed-off-by: Grant Linville <[email protected]>
|
The tests are failing right now only because the download links for Node.js are apparently down |
Signed-off-by: Grant Linville <[email protected]>
g-linville
dismissed stale reviews from StrongMonkey and thedadams
via
c085b9b
This implements the concept of "stacked" credential contexts. (Reminder: a credential context is basically just a namespace for credentials in the creds store.)
For more details on how this works, see the section that I added to the docs in this PR.