Skip to content
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: Query sharing s3 #974

Open
wants to merge 5 commits into
base: feat/query-result-sharing
Choose a base branch
from

Conversation

gaurpulkit
Copy link
Collaborator

  • Modifying the existing Query Result Sharing PR by using a pre-signed s3 url to send the data directly to the user's s3 server instead of storing the data in Altimate's database.

@mdesmet
Copy link
Contributor

mdesmet commented Mar 10, 2024

I think the point is to have both ways. We should offer the choice to the user to either use s3 bucket or use our saas.

@anandgupta42: Can you confirm?

@gaurpulkit gaurpulkit marked this pull request as ready for review March 18, 2024 03:43
@@ -219,25 +219,53 @@ export class QueryResultPanel implements WebviewViewProvider {
window.withProgress(
{
location: ProgressLocation.Notification,
title: "Uploading data to Altimate backend",
title: "Generating Shareable URL",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add telemetry

cancellable: false,
},
async () => {
try {
const result = await this.altimate.shareQueryResult(message);
if (result?.share_url) {
await env.clipboard.writeText(result.share_url);
if (result?.signed_url) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try early returns for better readability. ex:

if (!result?.signed_url){
window.showErrorMessage("Error generating signed url");
}

window.showInformationMessage(
                      "Generated signed url. Uploading data...",
                    );
                    ....

result.signed_url,
message,
);
if (uploadResponse.status === 200) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

early returns

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also http related stuff shouldn't leak into implementations.

ok: boolean;
share_url: string;
};
if (verifyData.ok) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

early returns

Copy link
Collaborator

@saravmajestic saravmajestic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some cleanup needed

@saravmajestic saravmajestic removed their request for review July 25, 2024 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants