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(core/services-gcs): support user defined metadata #5276

Merged
merged 14 commits into from
Nov 6, 2024

Conversation

jorgehermo9
Copy link
Contributor

@jorgehermo9 jorgehermo9 commented Nov 3, 2024

Relates to #4842

Please do not merge yet, I'm not sure this works.

I didn't see the CI for GCS behaviour tests. Im not sure that this PR is ok, as I do not have a GCP account and couldn't find out how to test it locally. Tried it with https://github.com/fsouza/fake-gcs-server and I also couldn't figure it out. Also, I can't get the gcs behavior tests running in local
image

Is the behaviour test plan ok? https://github.com/apache/opendal/actions/runs/11652657364/job/32444123433#step:3:39

It seems that the gcs service is being filtered out in this condition

if not os.getenv("GITHUB_HAS_SECRETS") == "true":

Maybe a maintainer has to manually run that check?

@jorgehermo9 jorgehermo9 marked this pull request as ready for review November 3, 2024 16:02
@jorgehermo9 jorgehermo9 requested a review from Xuanwo as a code owner November 3, 2024 16:02
@Xuanwo
Copy link
Member

Xuanwo commented Nov 3, 2024

I didn't see the CI for GCS behaviour tests.

Thank you, @jorgehermo9, for working on this.

We do have tests for GCS, but GitHub has security reasons for not allowing PRs from forks to use them. (This can be addressed once you become an OpenDAL committer!)

I will try to trigger the test for you.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @jorgehermo9, most changes LGTM, I will trigger a CI for it.

core/src/services/gcs/core.rs Show resolved Hide resolved
core/src/services/gcs/core.rs Show resolved Hide resolved
core/src/services/gcs/core.rs Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Nov 5, 2024

See #5284

@Xuanwo
Copy link
Member

Xuanwo commented Nov 5, 2024

Nice work!

image

@jorgehermo9
Copy link
Contributor Author

Hi @Xuanwo so nice! Thank you very much.

I'm open for any other suggestions!

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Perfect. Thank you @jorgehermo9 for this great PR!

@Xuanwo Xuanwo merged commit f547276 into apache:main Nov 6, 2024
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants