search

openmethane / openmethane-prior

Method to calculate a gridded, prior emissions estimate for methane across Australia
Apache License 2.0
4 stars 0 forks source link

Update GitHub secrets for CDS API #15

Closed crdanielbusch closed 5 months ago

crdanielbusch commented 5 months ago

The environment variable in the GitHub secret will generate the configuration file .cdsapirc for the download via the CDS API in omGFASEmis.py. At the moment, the configuration file is linked to my account.

Follow the instructions here

https://cds.climate.copernicus.eu/api-how-to

and replace user-id and API-key in the Github secret.

aethr commented 5 months ago

I've updated the CDSAPIRC secret with credentials from a new account registered to TSI.

GitHub doesn't let you edit secrets via the UI, it only lets you replace them, so I wasn't able to see what the format of the previous value was, but I'm assuming it was just the format of .cdsapirc.

aethr commented 5 months ago

Not sure exactly how to test, but I'm re-running the latest checks in #14 as this seems to use the secret. Hopefully if those checks pass, we can close this issue.

https://github.com/openmethane/openmethane-prior/actions/runs/9283109786/job/25577849350

aethr commented 5 months ago

This was not successful, due to a 401 error.

:facepalm: I created an account and added the credentials for CDS, not ADS. Have now created an ADS account and accepted the terms on the GFAS data set, and updated the secret. Re-running the job again.

aethr commented 5 months ago

Tests are now passing after updating with the ADS credentials: https://github.com/openmethane/openmethane-prior/actions/runs/9283109786

aethr commented 5 months ago

Re-opening since it looks like we're using a CDSAPI_KEY secret in https://github.com/openmethane/setup-wrf/pull/11, which I have not updated.

@lewisjared @crdanielbusch:

  1. Are these CDS API keys or ADS API keys?

If they're for ADS I'd suggest changing the name to ADSAPI_KEY. I know the tool is called "cdsapi", but CDS and ADS have separate accounts and APIs. If we're accessing the ADS API, I think it would be better to be explicit.

This already caused me some confusion when resolving this issue, as I followed the link in the description and created a CDS account and it took me a while to understand why the API key wasn't working.

  1. Would it make more sense to create an Organization secret for this and use it consistently across all the repos?
lewisjared commented 5 months ago

Yes, an organisation secret would be great here. I don't mind what the secret is called, but the environment variable has to be named CDSAPI_KEY. We can update the prior to use this environment variable instead of the whole .cdsapirc file

lewisjared commented 5 months ago

Splitting the name of the secret depending on if it is ADS or CDS is a good idea as there may be a case where we need both

aethr commented 5 months ago

@lewisjared I might avoid the secret name CDSAPI_KEY since that already has a meaning within the cdsapi package and we want to avoid ambiguity. What do we think of:

aethr commented 5 months ago

From reading https://github.com/ecmwf/cdsapi/blob/64a5df00eb086cd9389ae602d8d7f456cd9e4be2/cdsapi/api.py#L56 it seems that the format for CDSAPI_KEY is identical to the key value in .cdsapirc file, which is:

That's the format I'll use for the new Organization secrets.

aethr commented 5 months ago

Those secrets have been created.

@lewisjared I'll assign this issue to you to update https://github.com/openmethane/setup-wrf/pull/11 to use the new secrets and then remove the repo secret.

crdanielbusch commented 5 months ago

This already caused me some confusion when resolving this issue, as I followed the link in the description and created a CDS account and it took me a while to understand why the API key wasn't working.

Sorry @aethr for sending you down the CDS route! I wasn't aware CDS and ADS are not the same thing. I copied the link from the metadata in the cdsapi python package.

lewisjared commented 5 months ago

The old secret has been removed

aethr commented 5 months ago

Sorry @aethr for sending you down the CDS route! I wasn't aware CDS and ADS are not the same thing. I copied the link from the metadata in the cdsapi python package.

@crdanielbusch nah mate, not your fault at all. It is so confusing how they don't share accounts between the two and they look identical when you're logged in! Calling the tool "cdsapi" definitely doesn't help. We're all on the same page now and we've made it easy for future engineers to understand the difference. :)

aethr commented 5 months ago

@lewisjared @crdanielbusch is it safe to delete the CDSAPIRC secret in this repo?