-
Notifications
You must be signed in to change notification settings - Fork 1k
Allow all SKUs to use BYOK #242
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
base: main
Are you sure you want to change the base?
Conversation
@@ -130,8 +130,41 @@ export function resolveModelInfo(modelId: string, providerName: string, knownMod | |||
}; | |||
} | |||
|
|||
/** |
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.
No need for comments if they were not there before. I know some devs prefer code with 0 comments.
} | ||
|
||
// For business and enterprise users, require Editor Preview Features to be enabled | ||
if (copilotToken.copilotPlan === 'business' || copilotToken.copilotPlan === 'enterprise') { |
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.
No need to hard code 'business', 'enterprise'. There is probably some enum somewhere you can use.
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 actually don't think there is an enum - this value is currently defined as a string literal union:
get copilotPlan(): 'free' | 'individual' | 'individual_pro' | 'business' | 'enterprise' { |
|
||
// For business and enterprise users, require Editor Preview Features to be enabled | ||
if (copilotToken.copilotPlan === 'business' || copilotToken.copilotPlan === 'enterprise') { | ||
return copilotToken.isEditorPreviewFeaturesEnabled(); |
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.
0 enterprises have preview features enabled.
So I would not go down this route. For now I would just enable for insiders, and if GH wants a policy to disable once they deliver we can return that boolean. And I hope that is ENABLED by default.
Thanks for this PR. I left some comments. |
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.
Not opposed to the JSDoc comment but if you do end up removing it, just inline some of the details.
return true; | ||
} | ||
|
||
// Allow for individual users without preview features gating (backward compatibility) |
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.
// Allow for individual users without preview features gating (backward compatibility) | |
// Allow for individual users without preview feature-gating (backward compatibility) |
* | ||
* @param copilotToken The user's Copilot token (without the actual token value) | ||
* @param capiClientService Service to check if running on GitHub Enterprise | ||
* @returns true if BYOK should be enabled for this user |
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.
* @returns true if BYOK should be enabled for this user | |
* @returns whether BYOK should be enabled for this user |
} | ||
|
||
// For business and enterprise users, require Editor Preview Features to be enabled | ||
if (copilotToken.copilotPlan === 'business' || copilotToken.copilotPlan === 'enterprise') { |
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 actually don't think there is an enum - this value is currently defined as a string literal union:
get copilotPlan(): 'free' | 'individual' | 'individual_pro' | 'business' | 'enterprise' { |
…te UI messages for clarity
@isidorn OK, give it another look. Just cleaned up the logic more. :) |
// One‑time disclaimer: make it clear that BYOK models are external and not covered by Copilot guarantees. | ||
if (!this._byokDisclaimerShown) { | ||
this._byokDisclaimerShown = true; // ensure we only show once per session | ||
const detail = 'Models you configure here are provided by external services that you choose. Your prompts and code may be sent to those services and are subject to their terms. GitHub Copilot\'s data handling, compliance, and quality guarantees do NOT apply to BYOK models.'; |
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.
You might need to improve this string to be Localized. I am not 100% sure how to do it from this extension, but ask agent mode.
if (!this._byokDisclaimerShown) { | ||
this._byokDisclaimerShown = true; // ensure we only show once per session | ||
const detail = 'Models you configure here are provided by external services that you choose. Your prompts and code may be sent to those services and are subject to their terms. GitHub Copilot\'s data handling, compliance, and quality guarantees do NOT apply to BYOK models.'; | ||
const learnMore = 'Learn More'; |
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.
Same for all these other strings. I think all user facing strings need a special syntax to be localized.
@@ -126,6 +127,25 @@ export class BYOKContrib extends Disposable implements IExtensionContribution { | |||
} | |||
|
|||
private async registerModelCommand() { | |||
// One‑time disclaimer: make it clear that BYOK models are external and not covered by Copilot guarantees. | |||
if (!this._byokDisclaimerShown) { | |||
this._byokDisclaimerShown = true; // ensure we only show once per session |
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.
How often is the registerModelCommand called?
If it is called only once the user adds a BYOK model - then I think this would work.
Left some comments. Did you get a chance to test this out? If yes, can you share a screenshot / gif so I see the flow to potentially provide more feedback. |
This PR removes a license check that was in place for "bring your own key" which previously restricted use to Copilot Free and Individual SKUs.
This also shows a warning the first time you add a BYOK model in VS Code.