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: support google cloud storage as das #2643

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

renlulu
Copy link
Contributor

@renlulu renlulu commented Sep 6, 2024

No description provided.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Sep 6, 2024
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

Nice work so far! It just needs some additional testing and addressing my comments, which are fairly minor.

das/google_cloud_storage_service.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
das/google_cloud_storage_service.go Outdated Show resolved Hide resolved
@renlulu
Copy link
Contributor Author

renlulu commented Sep 7, 2024

Proof of working:
image
image

@renlulu renlulu force-pushed the feat/das-google-cloud branch 2 times, most recently from a9aee5f to 0750428 Compare September 9, 2024 19:52
das/google_cloud_storage_service.go Outdated Show resolved Hide resolved
das/google_cloud_storage_service.go Outdated Show resolved Hide resolved
cmd/genericconf/config.go Outdated Show resolved Hide resolved
cmd/genericconf/config.go Outdated Show resolved Hide resolved
@renlulu
Copy link
Contributor Author

renlulu commented Sep 20, 2024

@Tristan-Wilson Hey can we follow up here?

Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

Looking good, minor comment only now.

cmd/genericconf/config.go Outdated Show resolved Hide resolved
@renlulu
Copy link
Contributor Author

renlulu commented Oct 2, 2024

Looking good, minor comment only now.

Sorry i just saw the comment, I adjust the PR per your request.

Tristan-Wilson
Tristan-Wilson previously approved these changes Oct 3, 2024
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

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

LGTM. You will need to wait now for one of my colleagues to give this the design-reviewed tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants