-
Notifications
You must be signed in to change notification settings - Fork 2
Adding the platform selector and refactoring the new credential code #2688
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
Conversation
extensions/vscode/package.json
Outdated
"positPublisher.enableConnectCloud": { | ||
"markdownDescription": "Controls whether Connect Cloud support is available. This is an experimental feature and it is currently unstable. Please wait until it reaches alpha stage to turn it on.", | ||
"type": "boolean", | ||
"default": false |
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.
New configuration option to enable feature flagging for adding Connect Cloud support
}, | ||
"posit-publisher-posit-logo": { | ||
"description": "posit logo icon", | ||
"default": { | ||
"fontPath": "dist/posit-publisher-icons.woff2", | ||
"fontCharacter": "\\f102" | ||
} |
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.
New Posit logo icon used for both "Posit Connect" and "Posit Connect Cloud" until new product specific logos become available for general 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.
Very nice!
// Fetch the list of all available snowflake connections | ||
export const fetchSnowflakeConnections = async (serverUrl: string) => { | ||
let connections: SnowflakeConnection[] = []; | ||
let connectionQuickPicks: QuickPickItemWithIndex[]; | ||
|
||
try { | ||
const api = await useApi(); | ||
const connsResponse = await api.snowflakeConnections.list(serverUrl); | ||
connections = connsResponse.data; | ||
connectionQuickPicks = connections.map((connection, i) => ({ | ||
label: connection.name, | ||
index: i, | ||
})); | ||
} catch (error: unknown) { | ||
if (isAxiosErrorWithJson(error)) { | ||
throw error; | ||
} | ||
const summary = getSummaryStringFromError( | ||
"newCredentials, snowflakeConnections.list", | ||
error, | ||
); | ||
window.showErrorMessage( | ||
`Unable to query Snowflake connections. ${summary}`, | ||
); | ||
throw error; | ||
} | ||
|
||
if (!connectionQuickPicks.length) { | ||
const msg = `No working Snowflake connections found for ${serverUrl}. Please configure a connection in your environment before creating a credential.`; | ||
window.showErrorMessage(msg); | ||
throw new Error(msg); | ||
} | ||
|
||
return { connections, connectionQuickPicks }; | ||
}; |
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 a very small attempt at refactoring common code used in the multi-stepper newCredential.ts
and newDeployment.ts
files.
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 doing this! Having shared code is so much better it is greatly appreciated.
platformName = pick.label as PlatformName; | ||
state.lastStep = thisStepNumber; | ||
|
||
return (input: MultiStepInput) => inputServerUrl(input, state); |
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.
In the very near future this will be the point where we either go the "Posit Connect Cloud" route or the "Posit Connect" route. For now we are going the "Posit Connect" route always since we are still working on the "Posit Connect Cloud" integration bits.
if (isConnect(serverType)) { | ||
return (input: MultiStepInput) => inputAPIKey(input, state); | ||
} | ||
|
||
if (isSnowflake(serverType)) { | ||
return (input: MultiStepInput) => inputSnowflakeConnection(input, state); | ||
} |
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 one example of code refactor to "jump" directly to applicable steps rather than visiting every step and "falling through" when it does not apply.
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.
Thats a great refactor. Thank you for calling it out here I would have been confused otherwise why this change was made. Perhaps in the future we can separate out these quick improvements / refactors into separate PRs aside from the new feature work.
// Two credentials for the same URL is not allowed so clear the default if one is found | ||
if ( | ||
currentURL !== "" && | ||
findExistingCredentialByURL(credentials, currentURL) |
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.
Bringing parity between newCredential.ts
and newDeployment.ts
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.
Thank you! I hope we are able to refactor these in the future to more easily share functionality as you did with some of the Snowflake logic.
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.
That's my plan, but it will come in later PRs.
return Promise.resolve({ | ||
message: `Error: Invalid URL (${getMessageFromError(e)}).`, | ||
message: `Unexpected error within NewCredential::inputSeverUrl.finalValidation: ${JSON.stringify(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.
Bringing parity between newCredential.ts
and newDeployment.ts
const err = testResult.data.error; | ||
if (err) { | ||
if (err.code === "errorCertificateVerification") { | ||
return Promise.resolve({ | ||
message: `Error: Invalid URL (unable to validate connectivity with Server URL - API Call result: ${testResult.status} - ${testResult.statusText}).`, | ||
message: `Error: URL Not Accessible - ${err.msg}. If applicable, consider disabling [Verify TLS Certificates](${openConfigurationCommand}).`, | ||
severity: InputBoxValidationSeverity.Error, | ||
}); | ||
} | ||
if (testResult.data.error) { | ||
return Promise.resolve({ | ||
message: `Error: Invalid URL (${testResult.data.error.msg}).`, | ||
severity: InputBoxValidationSeverity.Error, | ||
}); | ||
} | ||
|
||
if (testResult.data.serverType) { | ||
serverType = testResult.data.serverType; | ||
} | ||
} catch (e) { | ||
return Promise.resolve({ | ||
message: `Error: Invalid URL (unable to validate connectivity with Server URL - ${getMessageFromError(e)}).`, | ||
message: `Error: Invalid URL (unable to validate connectivity with Server URL - ${getMessageFromError(err)}).`, |
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.
Bringing parity between newCredential.ts
and newDeployment.ts
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.
Overall this looks great. I tried setting the Connect Cloud setting and playing around with the new Credential and Deployment flows and the new platform selector works great. The new logo looks good too 👌
I do have the worry of moving this into main
which we try to keep release-able (if possible). I think we can get this merged without any worry about releasing the code if we remove the config item and set an internal feature flag in the area I outlined.
I had a couple other questions in here too. I did a full careful review so we should be able to get this merged in quickly once those are addressed or discussed.
Wanted to note that I really appreciated the refactor here, but it lead to a lot of whitespace changes in the multiStepInputs
that made the PR a bit tricky to review.
I want to keep doing refactors like that when we notice areas for improvement, but we can split them up a bit when making commits / PRs to make them easier to review, verify, and get into main
. Separating the refactor and platform selection work here would had been very helpful from my end.
}, | ||
"posit-publisher-posit-logo": { | ||
"description": "posit logo icon", | ||
"default": { | ||
"fontPath": "dist/posit-publisher-icons.woff2", | ||
"fontCharacter": "\\f102" | ||
} |
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.
Very nice!
// Fetch the list of all available snowflake connections | ||
export const fetchSnowflakeConnections = async (serverUrl: string) => { | ||
let connections: SnowflakeConnection[] = []; | ||
let connectionQuickPicks: QuickPickItemWithIndex[]; | ||
|
||
try { | ||
const api = await useApi(); | ||
const connsResponse = await api.snowflakeConnections.list(serverUrl); | ||
connections = connsResponse.data; | ||
connectionQuickPicks = connections.map((connection, i) => ({ | ||
label: connection.name, | ||
index: i, | ||
})); | ||
} catch (error: unknown) { | ||
if (isAxiosErrorWithJson(error)) { | ||
throw error; | ||
} | ||
const summary = getSummaryStringFromError( | ||
"newCredentials, snowflakeConnections.list", | ||
error, | ||
); | ||
window.showErrorMessage( | ||
`Unable to query Snowflake connections. ${summary}`, | ||
); | ||
throw error; | ||
} | ||
|
||
if (!connectionQuickPicks.length) { | ||
const msg = `No working Snowflake connections found for ${serverUrl}. Please configure a connection in your environment before creating a credential.`; | ||
window.showErrorMessage(msg); | ||
throw new Error(msg); | ||
} | ||
|
||
return { connections, connectionQuickPicks }; | ||
}; |
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 doing this! Having shared code is so much better it is greatly appreciated.
if (isConnect(serverType)) { | ||
return (input: MultiStepInput) => inputAPIKey(input, state); | ||
} | ||
|
||
if (isSnowflake(serverType)) { | ||
return (input: MultiStepInput) => inputSnowflakeConnection(input, state); | ||
} |
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.
Thats a great refactor. Thank you for calling it out here I would have been confused otherwise why this change was made. Perhaps in the future we can separate out these quick improvements / refactors into separate PRs aside from the new feature work.
// Two credentials for the same URL is not allowed so clear the default if one is found | ||
if ( | ||
currentURL !== "" && | ||
findExistingCredentialByURL(credentials, currentURL) |
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.
Thank you! I hope we are able to refactor these in the future to more easily share functionality as you did with some of the Snowflake logic.
@dotNomad Thank you for reviewing my PR! I'm sorry about the white spaces. I know those can be annoying when reviewing a PR and I'll try to make an effort to separate code changes more intentionally moving forward. I always use the "Hide whitespace" feature in GitHub and that helps me deal with the white spaces. I wanted to ask if you used it and still found it too heavy on the changes? Or maybe you did not use it. Just curious. Thanks again! ![]() |
@dotNomad I just pushed some changes to address your feedback. It's ready for round 2 of review, thank you! |
I use it sparingly. I toggled it on for this PR since there were some nesting changes, but I still saw quite a few changes on the
Awesome taking a look! 👀 |
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.
Thank you for those changes to the config. This is looking great 👍
I noted some potential improvements for avoiding the enum dance, but due to type safety achieving it is a bigger lift and not one I'd recommend doing inside of an already in progress PR
This PR introduces the initial platform selector needed for adding Connect Cloud support into the Publisher plugin. The platform selector is hidden by default behind the
enableConnectCloud
config option and it is not meant to be used yet at all since there are more changes needed before it can be used. If theenableConnectCloud
configuration option were to be tuned on by a user, they would see the platform selector with two options ("Posit Connect Cloud" & " Posit Connect") but selecting either option would still take them through the Connect flow for adding a new credential, so for now all roads lead to Rome.The multi-stepper code has been refactored to "jump" steps accordingly with the value of the
serverType
instead of "falling through" the non-applicable steps when adding a new credential. These changes were made to facilitate adding support of new platforms into the plugin such as Connect Cloud.A new icon has been added for the Posit logo using the
npm run build-font
script to generate the appropriate icon-font and CSS file to make the icon available to the multi-stepper.Intent
Resolves #2685
Type of Change
Approach
As of today the Publisher plugin only supports deployments onto a Posit Connect server running on user hosted infrastructure or a Posit Connect server running on Snowflake's SPCS infrastructure. Each of these have different credential needs hence their
serverType
is different but both are supporting the same platform: Posit Connect.I have introduced a platform selector so new platforms such as Posit Connect Cloud can be supported and differentiated. For all purposes the platform selector will treat a Posit Connect server running on user hosted infrastructure the same as a Posit Connect server running on Snowflake's SPCS infrastructure. In order words those two flavors of Connect will be a single choice in the platform selector: "Posit Connect". However, they will be treated differently when adding a new credential once the server URL has been provided as it is implemented today.
New platforms such as Posit Connect Cloud will have their own entry in the platform selector so all the different multi-stepper steps can be properly handled. Adding the platform selector and refactoring the multi-stepper code to "jump" directly to applicable steps will simplify adding new platforms to the plugin.
User Impact
These changes will not have any user impact since they are hidden by default behind a configuration option and the E2E tests have been run successfully against this branch.
Automated Tests
Unit tests have been added for the testable parts of this change. No new E2E tests have been added yet for the platform selector since it is hidden by default behind a configuration option. New E2E tests will be added later on when more bits are working for supporting multiple platforms.
Directions for Reviewers
Validation of the existing flow for adding a new credential should be tested and it is expected to be working correctly including adding a new credential separately from adding a new deployment or as part of adding a new deployment.
Checklist