feat(core): support AICORE_SERVICE_KEY env var for credentials and rename env vars - #58
feat(core): support AICORE_SERVICE_KEY env var for credentials and rename env vars#58yamaceay wants to merge 9 commits into
Conversation
2b882f6 to
0271d5b
Compare
| ) | ||
| from ai_core_sdk.helpers.constants import (AI_CORE_PREFIX, HOME_PATH_ENV_VAR, PROFILE_ENV_VAR, VCAP_SERVICES_ENV_VAR, | ||
| VCAP_AICORE_SERVICE_NAME, CONFIG_FILE_ENV_VAR) | ||
| VCAP_AICORE_SERVICE_NAME, CONFIG_FILE_ENV_VAR, SERVICE_KEY_ENV_VAR) |
There was a problem hiding this comment.
[pp] Not related to this PR, but possibly a nice new backlog item: I noticed that the variables are imported in a different order here. So this does not seem to be covered by linting. Possibly worth improving, if you think so too.
ZhongpinWang
left a comment
There was a problem hiding this comment.
First time reviewing python SDK :)
Some comments can be addressed later when doing the actual refactoring / cleaning up.
| ) from exc | ||
|
|
||
| def _get(cv: CredentialsValue) -> Optional[str]: | ||
| if not cv.vcap_key: |
There was a problem hiding this comment.
[req] (maybe for the future) After struggling and looking into the codebase, I finally understand what is a vcap_key........ It is a JSON object access path starting from credentials stored in the form of a tuple...
I know it is not super relevant to this PR. But refactoring needed... And allow me to just put my thoughts here as a reference. I added this comment to the refactoring BLI.
I think we should really consider renaming few stuff.
- "Key" means a single property name
- "Access Path" means
obj.credentials.clientid - "Value" means the content of that key value pair, what we get when calling
get(key)
Also it is pretty hard to understand what CredentialsValue actually means. We call the whole object inside VCAP_SERVICES['aicore'][0] a service binding.
And why do we define object schema in such a complicated way. Can we not just define the data class of the credentials object such as AiCoreCredentials? Then transform_fn takes an AI CORE service binding object and map it to AiCoreDestination? (I used the term Destination as it is pretty much the transformed object)
@mwien to give you an overview of how VCAP_SERVICES could actually look like:
{
"VCAP_SERVICES": {
"aicore": [
{
"label": "aicore",
"provider": null,
"plan": "<plan>",
"name": "default_aicore",
"tags": [],
"instance_guid": "<some uuid>",
"instance_name": "default_aicore",
"binding_guid": "<some_other_uuid>",
"binding_name": null,
"credentials": {
"serviceurls": {
"AI_API_URL": "https://api.ai.......ml.hana.ondemand.com"
},
"appname": "<appname>",
"clientid": "<clientid>",
"clientsecret": "<clientsecret>",
"identityzone": "<subdomain of the subaccount>",
"identityzoneid": "<tenant id / subaccount id>",
"url": "<auth_url>"
},
"syslog_drain_url": null,
"volume_mounts": []
}
],
// ...
}
}There was a problem hiding this comment.
I also got super confused by this, but I also believe that this might be because I am not used to python as much.
| [tool.pytest.ini_options] | ||
| testpaths = ["tests"] | ||
| norecursedirs = ["integration_tests"] | ||
| # Prevent pytest-dotenv from loading the repo-root .env (which contains real credentials) |
There was a problem hiding this comment.
[q/req] Is there anywhere documented that we should store .env in the root (for development I guess)? Or is it just your local setup, and you stored .env at the root?
Later we will have a sample server folder / package, which will be used for trying out purposes and we can put .env in that folder? cc @mwien
There was a problem hiding this comment.
In the local setup, I ran into errors running the unit tests since .env interfered with the mock credentials. This was meant rather for local development, I will investigate further.
There was a problem hiding this comment.
I am just not entirely sure if we should or must put .env at root. Not sure what python colleagues previously used to do.
There was a problem hiding this comment.
For reference: uv run pytest packages/core/tests without env-files override yields 10 errors in core package:
============================================================================ short test summary info =============================================================================
FAILED packages/core/tests/ai_core_client/test_ai_core_v2_client.py::TestAICoreV2Client::test_from_env - AssertionError: expected call not found.
...
FAILED packages/core/tests/ai_core_client/test_credentials.py::TestConfigHandling::test_fetch_credentials_from_vcap_services_with_x509_env_var - AssertionError: 'https://api.ai.<redacted>.ml.hana.ondemand.com/v2' != 'vcap-api-url/v2'
=================================================================== 10 failed, 71 passed, 3 warnings in 1.05s ====================================================================
make: *** [test] Error 1There was a problem hiding this comment.
My idea was to not put .env locally for development at root. Then we don't need to exclude any env files.
.env will be put as part of the new sample server folder once Marcel finished creating it.
Or are we writing any .env in the workflow which causes the issue?
Co-authored-by: Zhongpin Wang <zhongpin.wang@sap.com>
90d6eb6 to
6861739
Compare
| def _get_nested_safe(data: Dict, keys) -> Optional[Any]: | ||
| try: | ||
| return get_nested_value(data, keys) | ||
| except KeyError: |
There was a problem hiding this comment.
[pp] Maybe add a warning log here? I in general don't feel super comfortable to silent some exceptions.
There was a problem hiding this comment.
I am not really sure whether this should be warning. If we resolve credentials one by one until VCAP, it will always emit warning in the production. Maybe debug makes more sense. What do you think?
ZhongpinWang
left a comment
There was a problem hiding this comment.
Looks much better now! Thanks for your effort 👍
Co-authored-by: Zhongpin Wang <zhongpin.wang@sap.com>
099e8da to
4d72a82
Compare
This PR aims to align the authentication conventions of core and gen packages more closely. Not directly applicable to base, since the definition of AI API client must remain unchanged. New unit tests are added + env vars are removed from unit tests.