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

adds scoped values credential to support multiple endpoints. #58

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

Conversation

ghyatzo
Copy link
Contributor

@ghyatzo ghyatzo commented Jan 15, 2025

Hello,

I had wrote my script for downloading CDS data, before checking if it existed already.
Funnily enough our implementations are eerily similar.
There are some slight differences that I would like to propose as improvements/additions to this package so we can have a single point.

This is the first of 3 pull requests, one for each proposal so they can be addressed independently.

Ok with that out of the way:
I would like to propose the use of scoped values for the API credentials. As well as a bit better credential retrieval mechanism that also checks for environmental variables.

This allows to use different credentials for each request, for example:

cred1 = CDScredentials("path/to/first/set/of/credentials")
cred2 = CDScredentials("path/to/second/set/of/credentials")

with( auth => cred1) do
    CDSAPI.retrieve(name, request, target)
end

with( auth => cred2) do
    # makes the requests with a different token
    CDSAPI.retrieve(name, request, target)
end

# If none are provided, we look for credentials in the environmental variables
# and then in the default "~/.cdsapirc"
CDSAPI.retrieve(name, request, target)

Copy link
Member

@juliohm juliohm 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 for the suggested improvement @ghyatzo , you can find review comments attached.

src/CDSAPI.jl Outdated Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
src/CDSAPI.jl Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 15, 2025

I'll add some examples of this new usage in the readme later

src/CDSAPI.jl Outdated
Comment on lines 6 to 12
if VERSION >= v"1.11"
using Base.ScopedValues
elseif VERSION >= v"1.8"
import Pkg
Pkg.activate(".")
Pkg.add("ScopedValues")
using ScopedValues
end
Copy link
Member

Choose a reason for hiding this comment

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

The idea is to run these commands in the source code to modify the Project.toml of the package. There is no problem in adding ScopedValues.jl as a dependency.

Copy link
Contributor Author

@ghyatzo ghyatzo Jan 16, 2025

Choose a reason for hiding this comment

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

Ok, so then just remove the Pkg.add("ScopedValues") and add it normally

@juliohm
Copy link
Member

juliohm commented Jan 16, 2025 via email

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 16, 2025

OK, I've rebased onto master to have the other PRs.
I'll add some tests and examples in the README

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 16, 2025

tests pass locally

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.

2 participants