-
Notifications
You must be signed in to change notification settings - Fork 172
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
Added new Cohere Cli plugin #334
base: main
Are you sure you want to change the base?
Conversation
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.
Hi @parthiv11, thanks for this submission. A couple of questions:
- Is this still a work in progress? I see some todo.
- Could you run
gofmt -w .
to address the linter errors? - The tests don't seem complete. Could you address them, please?
With this understanding, I'll mark this PR as in progress for now, but feel free to ping me once it's ready for a review.
Also, this is a hackathon entry, yes? |
If it is a Hackathon entry - be sure to write a blog post on Hashnode tagged with 1Password and BuildWith1Password. Also, be sure to use Hashnode tags, not just "#1Password". Here's a guide. :) |
@arunsathiya Added tests for cohere plugins |
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.
Thanks for this contribution! Looks good, for the most part 😄
plugins/cohere/credentials.go
Outdated
provision.Filename("config"), | ||
provision.AtFixedPath("~/.command/")), |
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.
These two options, if I recall correctly, are not both taken into consideration.
I think you're looking for `provision.AtFixedPath("~/.command/config").
plugins/cohere/credentials.go
Outdated
if value, ok := in.ItemFields[fieldname.Authtoken]; ok { | ||
jwt = value | ||
} |
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.
If this value is not found, let's bail and return.
needsauth.NotForHelpOrVersion(), | ||
needsauth.NotWithoutArgs(), |
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 there may be other commands, such as co auth login
, that do not require authentication - could you have a look at what all of those may be?
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 is not yet resolved
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.
Almost there! 🚀 Please make sure to goimports
your files, looks like the lint is complaining.
Also, please have a look over the failing tests. |
Hey @parthiv11 , I see there hasn't been any activity on this PR in a bit, is there anything that we can help you with? We'd love to get this to the finish line together! 🚀 |
co-2023-07-12_12.28.50.mp4@hculea i think it is done |
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.
Looks great, your functional test video helps a lot! Only a few more comments related to the code.
9bb9ebf
to
a991242
Compare
"context" | ||
|
||
"encoding/json" | ||
"github.com/1Password/shell-plugins/sdk" |
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.
Let's sort imports on this file to make the linter pass.
plugins/cohere/credentials.go
Outdated
out.AddCandidate(sdk.ImportCandidate{ | ||
Fields: map[sdk.FieldName]string{ | ||
fieldname.URL: config.CurrentURL, | ||
fieldname.JWT: config.Contexts[config.CurrentURL].JWT, |
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.
Let's format this code so the linter can pass
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.
code lgtm mostly. I went ahead and closed all resolved comments. There are still very few open comments which need to be resolved first, as well as some formatting required for the GitHub linter action to pass.
@parthiv11 is this something you can take a look at?
@AndyTitu done |
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
Hi @parthiv11 ! It looks like not all the comments are resolved and the linter/tests still aren't passing. Are you able to fix those? Thank you again for your contribution! |
Overview
Added cohere cli shell plugin
Type of change
Related Issue(s)
How To Test
Changelog
Additional information