Skip to content

adds scoped values credential to support multiple endpoints.#58

Merged
juliohm merged 31 commits intoJuliaClimate:masterfrom
ghyatzo:scoped-auth
Feb 24, 2025
Merged

adds scoped values credential to support multiple endpoints.#58
juliohm merged 31 commits intoJuliaClimate:masterfrom
ghyatzo:scoped-auth

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.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 15, 2025

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

@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

@juliohm
Copy link
Member

juliohm commented Feb 11, 2025

@ghyatzo the tests are failing. Should we continue the work on this PR?

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Feb 12, 2025

Hi, we encountered this issue in a previous PR #60 (comment)

Have you tried locally? apparently the PR branches do not have access to the secrets in the CI so the fail I saw were there, but locally with my credentials this branch pass all tests (at least, last time I checked, don't know if something else changed in the mean time)

@juliohm
Copy link
Member

juliohm commented Feb 12, 2025

@ghyatzo there are pending review comments. I can try to run things locally again after they are considered.

renamed the scoped values to the uppercase variant
ghyatzo and others added 4 commits February 20, 2025 14:44
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
@ghyatzo
Copy link
Contributor Author

ghyatzo commented Feb 20, 2025

ok, sorry for the sloppy last few iterations, should have addressed the latest issues. I've merged master into the PR and tests pass locally.
Let me know if you need anything else

ghyatzo and others added 4 commits February 21, 2025 17:53
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
Enforce presence of key and url in file.
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
@juliohm
Copy link
Member

juliohm commented Feb 24, 2025

Tests are passing locally for me, but failing on GitHub Actions. Some of the build failures point to missing credentials.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Feb 24, 2025

@juliohm
Copy link
Member

juliohm commented Feb 24, 2025

see: #60 (comment) and: https://stackoverflow.com/questions/73866908/github-actions-main-repository-secret-not-picked-up-from-pull-request-build

I always forget. Sorry for the noise. Merging it.

@juliohm juliohm merged commit b48d3ed into JuliaClimate:master Feb 24, 2025
0 of 6 checks passed
@ghyatzo ghyatzo deleted the scoped-auth branch February 24, 2025 12:12
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

Comments