-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Reenable copilot #3931
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
Reenable copilot #3931
Conversation
|
Thank you for sharing this! Let me tag folks on the team for review. Just want to make sure in adding copilot back, that it does not introduce bugs like it did before! |
|
can we add co-pilot to the scenario tests if it works so we can see that it does/ |
@DOsinga are there are scenario tests today that work with oauth flows? In the goose-cli/src/scenario_tests directory, existing tests seem to use API tokens. |
I don't think so. would be good though |
The tricky part will be needing to replicate the interactions with the browser. I don't know of a good way to handle this in a headless fashion. |
|
Hi @DOsinga, I wonder if the Goose dev team would be open to merging this without tests for now, maybe placing it behind the |
|
What do you think, @DOsinga @michaelneale @alexhancock @zanesq ? I understand we want to be mindful of any issues. |
that sounds alright; could you have a look at how hard it would be to add a feature to the providers to mark them up as alpha/beta? there are frankly a whole bunch of providers that are, eh, undertested |
c098b76 to
9e06a39
Compare
Signed-off-by: Jack Wright <[email protected]>
12d75a6 to
bcdc926
Compare
Signed-off-by: Jack Wright <[email protected]>
There is also a bit of a chicken and egg problem here. A lot of corporate users are going to need to use goose through the github copilot support. It's a pretty common pattern to block all providers except copilot (e.g. legal compliance / contracts with Microsoft, etc). In my case, I am working on getting goose to be approved tech at my company and this is the only blocker. If we can get this over the line, we can increase the number of people using this feature and also contributing to project. |
This is exactly my case :) |
Same here. Rather than implement alpha/beta gating (if that's not already implemented) could we maybe rename the provider to have alpha in the title as an interim step? Or something similarly user-facing that denotes some level of instability without needing much implementation so that users like us can start using copilot again? :) |
DOsinga
left a comment
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 just merge it for now :)
|
Yay, happy to get this merged! |
Signed-off-by: Jack Wright <[email protected]> Co-authored-by: Jack Wright <[email protected]> Signed-off-by: Alex Rosenzweig <[email protected]>
Signed-off-by: Jack Wright <[email protected]> Co-authored-by: Jack Wright <[email protected]> Signed-off-by: Dorien Koelemeijer <[email protected]>
Since PR 3157 has been merged, we copilot should be re-enabled.