-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Support multiple organizations API key. #68
Conversation
open Quick-pick dialog and let the user pick the org
src/auth.ts
Outdated
private selectedOrgId: string | undefined; | ||
|
||
public getSelectedOrg() { | ||
return this.selectedOrgId; | ||
} |
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.
why hold the this id in 2 different places, shouldn't there be a single source of truth? ( probably here and not in api client )
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 completely agree. I chose this approach because it aligns with our existing method of managing user credentials.
When I was creating the auth service, my intention was to pass an instance of it to the API client to avoid this very issue. However, I received this comment
4c8a496
also saved selected org id in store
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 its up to you 👍🏻
this was the only important comment approving 👍🏻
src/get-environments.ts
Outdated
try { | ||
return apiClient.getEnvironments(organizationId); | ||
return apiClient.getEnvironments(); | ||
} catch (e) { | ||
console.log(e); |
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.
where do console logs go in a vs code extension?
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.
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 was just asking for general knowledge 👍🏻
inject auth service to api client
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.
LGTM 👍🏻
on login when there is more then one org connected to the auth key open Quick-pick dialog and let the user pick the org
note: I have conducted a manual QA, and everything appears to be in order, but I strongly recommend that one more person conduct manual QA before the release.