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

[R] Allow GcsFileSystem$create to accept a path to json_credentials #34421

Closed
amoeba opened this issue Mar 3, 2023 · 1 comment · Fixed by #34524
Closed

[R] Allow GcsFileSystem$create to accept a path to json_credentials #34421

amoeba opened this issue Mar 3, 2023 · 1 comment · Fixed by #34524

Comments

@amoeba
Copy link
Member

amoeba commented Mar 3, 2023

Describe the enhancement requested

As discovered in #33106, GcsFileSystem$create is documented to say it takes a path to a set of JSON credentials on disk. Right now, it only takes a character vector containing the JSON credentials.

So this works:

arrow::gs_bucket("my-gcs_bucket", json_credentials = readr::read_file("my-service_account.json"))

And this does not:

arrow::gs_bucket("my-gcs_bucket", json_credentials = "my-service_account.json")

I think both use cases should work so I think we should:

  1. Fix the documentation in [R] Documentation for json_credentials is misleading #33106 so it's clear both approaches work
  2. Expand the behavior of GcsFileSystem$create to handle a path to credentials or a string containing the credentials

Component(s)

R

@amoeba
Copy link
Member Author

amoeba commented Mar 3, 2023

take

paleolimbot pushed a commit that referenced this issue Mar 22, 2023
)

### Rationale for this change

Existing documentation for this argument was misleading.

### What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. `json_credentials` can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

### Are these changes tested?

Yes

### Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes #34421
Closes #33106
* Closes: #34421

Authored-by: Bryce Mecum <petridish@gmail.com>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@paleolimbot paleolimbot added this to the 12.0.0 milestone Mar 22, 2023
rtpsw pushed a commit to rtpsw/arrow that referenced this issue Mar 27, 2023
apache#34524)

### Rationale for this change

Existing documentation for this argument was misleading.

### What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. `json_credentials` can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

### Are these changes tested?

Yes

### Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes apache#34421
Closes apache#33106
* Closes: apache#34421

Authored-by: Bryce Mecum <petridish@gmail.com>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this issue May 15, 2023
apache#34524)

### Rationale for this change

Existing documentation for this argument was misleading.

### What changes are included in this PR?

A change in functionality, matching tests, and updated documentation are included. `json_credentials` can now either be a literal string containing credentials or a string containing a path to credentials. In the latter case, credentials will be automatically read in from the fileystem.

### Are these changes tested?

Yes

### Are there any user-facing changes?

Yes, though not breaking. This affects user-facing APIs and documentation and is both a bug fix and new functionality.

Closes apache#34421
Closes apache#33106
* Closes: apache#34421

Authored-by: Bryce Mecum <petridish@gmail.com>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants