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.

Sign up for GitHub

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

Jump to bottom

feat(gcs): allow unauthenticated requests #4965

Merged
merged 11 commits into from
Aug 7, 2024
Merged

feat(gcs): allow unauthenticated requests #4965

merged 11 commits into from
Aug 7, 2024

Conversation

jdockerty
Copy link
Contributor

@jdockerty jdockerty commented Aug 5, 2024 •
edited
Loading

Which issue does this PR close?

Closes #4963

What changes are included in this PR?

Introduction of:

  • allow_anonymous
  • disable_vm_metadata
  • disable_config_load

to GcsConfig and their corresponding plumbing within GcsCore.

Are there any user-facing changes?

Changes are internal only.

Xuanwo
Xuanwo reviewed Aug 6, 2024
View reviewed changes
core/src/services/gcs/backend.rs Outdated Show resolved Hide resolved
core/src/services/gcs/backend.rs Outdated Show resolved Hide resolved
@Xuanwo Xuanwo mentioned this pull request Aug 6, 2024
Closed
@jdockerty jdockerty marked this pull request as ready for review August 6, 2024 13:30
@jdockerty jdockerty mentioned this pull request Aug 7, 2024
Merged
@jdockerty
Copy link
Contributor Author

jdockerty commented Aug 7, 2024 •
edited
Loading

@Xuanwo Using this version of opendal locally for my iceberg-rust changes works with the DockerCompose setup. I believe this should be ready to take a look at.

Is there anything else you'd like me to include here?

I see there's the use of an AtomicBool for tracking previously loaded credentials within the S3 core, is that relevant to also do with this GCS change or better for another PR?

@Xuanwo
Copy link
Member

Xuanwo commented Aug 7, 2024

I see there's the use of an AtomicBool for tracking previously loaded credentials within the S3 core, is that relevant to also do with this GCS change or better for another PR?

That's fine to include in another PR.

Xuanwo
Xuanwo reviewed Aug 7, 2024
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks mostly good to me, just needs some semantic clarity.

core/src/services/gcs/backend.rs Outdated Show resolved Hide resolved
core/src/services/gcs/core.rs Outdated Show resolved Hide resolved
core/src/services/gcs/backend.rs Outdated Show resolved Hide resolved
Xuanwo
Xuanwo approved these changes Aug 7, 2024
View reviewed changes
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.

Excellent PR and great communication! Happy to work with you.

@Xuanwo Xuanwo merged commit 0a3e98f into apache:main Aug 7, 2024
79 checks passed
@Xuanwo
Copy link
Member

Xuanwo commented Aug 7, 2024

Perhaps you might be interested in assisting with #4876 😆

@jdockerty jdockerty deleted the feat/gcs-allow-no-auth branch August 7, 2024 14:24
@jdockerty jdockerty mentioned this pull request Aug 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees
No one assigned
Labels
releases-note/feat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

new feature: allow GCP unauthenticated requests
2 participants
@jdockerty @Xuanwo