feat(core): support AICORE_SERVICE_KEY env var for credentials by yamaceay · Pull Request #58 · SAP/ai-sdk-python

yamaceay · 2026-08-12T15:08:57Z

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.

marikaner

Just adding my 2 cents. As I am a python noob, @mwien please review ;)

marikaner · 2026-08-13T06:28:35Z

 DEFAULT_HOME_PATH = os.path.join(os.path.expanduser('~'), '.aicore')
 HOME_PATH_ENV_VAR = f'{AI_CORE_PREFIX}_HOME'
 PROFILE_ENV_VAR = f'{AI_CORE_PREFIX}_PROFILE'
+SERVICE_KEY_ENV_VAR = f'{AI_CORE_PREFIX}_SERVICE_KEY'


[pp] I think AICORE_SERVICE_KEY would be a simpler name and easier to find throughout the code. That said, the other variables follow a different naming pattern and changing this would introduce an inconsistency. Merely personal perference though.

[q/pp] I see that AI_CORE_PREFIX is a constant equal to string AICORE.

Maybe a follow-up: Overall I find the current constant list a bit too complex.

I understand the idea behind "having constant strings defined as variable to avoid typo" methodology here. Perhaps follow what React does: Similar to using REACT_APP_ as prefix for all env var, we can rename all these env vars into

ENV_VAR_AICORE_HOME

ENV_VAR_AICORE_PROFILE

ENV_VAR_AICORE_SERVICE_KEY

...

It improves the auto completion experience.

Also I would prefer inline AI_CORE_PREFIX with AICORE. It is just six characters, and whenever it is used, it is combined with other stuff like {AI_CORE_PREFIX}_CERT_FILE_PATH which I think typing the rest part is more error-prone :)

It doesn't have to be part of this PR though. But since we might have a lot of things needing refactoring, I will create a refactoring BLI and collect some ideas incl. this one.

Btw, some old tradition for PR review:

q: Question (Something I am not sure about)
pp: Personal preference (Not a must but nice to have)
req: Request changes (This should be changed)

Some of us this kind of form to indicate the type of review comments for better communication.

marikaner · 2026-08-13T06:36:46Z

 )
 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)


[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.

marikaner · 2026-08-13T06:38:02Z

+            'url': 'https://sk-auth-url',
+            'serviceurls': {'AI_API_URL': 'https://sk-api-url'},
+        }
+        with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: json.dumps(service_key)}):


[req] I believe this should be AICORE_SERVICE_KEY?

Suggested change

with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: json.dumps(service_key)}):

with patch.dict(os.environ, {AICORE_SERVICE_KEY: json.dumps(service_key)}):

Also, if I am not mistaken, why do the tests pass? 🧐

@marikaner It was also quite confusing to me.. If I understand it right, SERVICE_KEY_ENV_VAR looks like the constant that stores AICORE_SERVICE_KEY as the value.

I suggested above to change those constant names in the future. At least to something like ENV_VAR_AICORE_SERVICE_KEY (if we want to avoid typing strings everywhere)

marikaner · 2026-08-13T06:40:58Z

+            f'{AI_CORE_PREFIX}_AUTH_URL': 'https://env-auth-url',
+            f'{AI_CORE_PREFIX}_BASE_URL': 'https://env-base-url',


[pp] If you're not checking those, maybe it is not needed to add them here?

marikaner · 2026-08-13T06:54:31Z

+    def _get(cv: CredentialsValue) -> Optional[str]:
+        if not cv.vcap_key:
+            return None
+        # vcap_key is e.g. ('credentials', 'clientid') — drop the 'credentials' prefix


[pp] I had a hard time understanding what this comment was trying to tell me. Information that helped me understand:

vcap_key is a tuple (due to the comment I somehow thought it could be "credentials" or "clientid" or so)

the tuple always starts with an element "credentials"

we're dropping the first element, not a "prefix"

This might be partially me as a python noob, but consider sharpening the wording a bit (at least something like that: "prefix" => leading "credentials" element).

marikaner · 2026-08-13T06:58:14Z

+docs = [
+    "sphinx<9.0.0",
+    "sphinxawesome-theme",
+]


[req] this is unrelated to this change, right?

mwien · 2026-08-13T08:15:06Z

               lambda cv: _str_or_none(os.environ.get(f'{AI_CORE_PREFIX}_{cv.name.upper()}'))),
+        *(
+            [Source(SERVICE_KEY_ENV_VAR, service_key_getter)]
+            if service_key_getter is not None else []


Maybe an option to keep this more consistent would be to have service_key_getter be a Callable instead of Optional[Callable] and have it always return None in case the env var doesn't exist. See comment above.

mwien · 2026-08-13T08:16:32Z

+    """
+    raw = os.environ.get(SERVICE_KEY_ENV_VAR)
+    if not raw:
+        return None


It could be an option to not return here but write an empty dict into service_key and then return a Callable instead of Optional[Callable]. Could make the code below more consistent.

mwien · 2026-08-13T08:18:59Z

    except KeyError:
        vcap_service = None

+    service_key_getter = _parse_service_key(credential_values)


If the change suggested in line 304 is done, this could be moved to an inline call to _parse_service_key there

ZhongpinWang

First time reviewing python SDK :)

Some comments can be addressed later when doing the actual refactoring / cleaning up.

ZhongpinWang · 2026-08-13T08:55:28Z

 DEFAULT_HOME_PATH = os.path.join(os.path.expanduser('~'), '.aicore')
 HOME_PATH_ENV_VAR = f'{AI_CORE_PREFIX}_HOME'
 PROFILE_ENV_VAR = f'{AI_CORE_PREFIX}_PROFILE'
+SERVICE_KEY_ENV_VAR = f'{AI_CORE_PREFIX}_SERVICE_KEY'


[q/pp] I see that AI_CORE_PREFIX is a constant equal to string AICORE.

Maybe a follow-up: Overall I find the current constant list a bit too complex.

I understand the idea behind "having constant strings defined as variable to avoid typo" methodology here. Perhaps follow what React does: Similar to using REACT_APP_ as prefix for all env var, we can rename all these env vars into

ENV_VAR_AICORE_HOME

ENV_VAR_AICORE_PROFILE

ENV_VAR_AICORE_SERVICE_KEY

...

It improves the auto completion experience.

Also I would prefer inline AI_CORE_PREFIX with AICORE. It is just six characters, and whenever it is used, it is combined with other stuff like {AI_CORE_PREFIX}_CERT_FILE_PATH which I think typing the rest part is more error-prone :)

It doesn't have to be part of this PR though. But since we might have a lot of things needing refactoring, I will create a refactoring BLI and collect some ideas incl. this one.

ZhongpinWang · 2026-08-13T09:49:27Z

 DEFAULT_HOME_PATH = os.path.join(os.path.expanduser('~'), '.aicore')
 HOME_PATH_ENV_VAR = f'{AI_CORE_PREFIX}_HOME'
 PROFILE_ENV_VAR = f'{AI_CORE_PREFIX}_PROFILE'
+SERVICE_KEY_ENV_VAR = f'{AI_CORE_PREFIX}_SERVICE_KEY'


Btw, some old tradition for PR review:

q: Question (Something I am not sure about)
pp: Personal preference (Not a must but nice to have)
req: Request changes (This should be changed)

Some of us this kind of form to indicate the type of review comments for better communication.

ZhongpinWang · 2026-08-13T10:57:30Z

+        ) from exc
+
+    def _get(cv: CredentialsValue) -> Optional[str]:
+        if not cv.vcap_key:


[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": [] } ], // ... } }

ZhongpinWang · 2026-08-13T11:10:32Z

    return str(value) if value else None


+def _parse_service_key(credential_values: List[CredentialsValue]) -> Optional[Callable[[CredentialsValue], Optional[str]]]:


[req] The function is called _parse_service_key. Based on its name, nobody would expect it to return a callable. Function name should really be speaking. The actual json parsing part is like a precondition to the function.

Also credential_values is never used. This function is reading SERVICE_KEY_ENV_VAR from env.

This function is more like create_service_key_getter or something.

[req] Consider separating the JSON parsing / validation part out into a different function.

ZhongpinWang · 2026-08-13T11:18:59Z

+    def _get(cv: CredentialsValue) -> Optional[str]:
+        if not cv.vcap_key:
+            return None
+        # vcap_key is e.g. ('credentials', 'clientid') — drop the 'credentials' prefix


Suggested change

# vcap_key is e.g. ('credentials', 'clientid') — drop the 'credentials' prefix

# `vcap_key` is a tuple representing the access path to properties such as

# `clientid` and `clientsecret` in the `aicore` service binding, e.g.

# `('credentials', 'clientid')`. Skip the leading path element `credentials` to

# access the nested value in the JSON object.

What about this.

ZhongpinWang · 2026-08-13T11:24:52Z

+<strong>SERVICE_KEY_ENV_VAR</strong> = 'AICORE_SERVICE_KEY'<br>
 <strong>VCAP_AICORE_SERVICE_NAME</strong> = 'aicore'<br>
 <strong>VCAP_SERVICES_ENV_VAR</strong> = 'VCAP_SERVICES'</td></tr></table>
 </body></html>


[pp] Consider not making unnecessary changes.

Also it is a good practice to always leave an empty line at the end of a file (coming from a unix world).

ZhongpinWang · 2026-08-13T11:26:29Z

+            'url': 'https://sk-auth-url',
+            'serviceurls': {'AI_API_URL': 'https://sk-api-url'},
+        }
+        with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: json.dumps(service_key)}):


@marikaner It was also quite confusing to me.. If I understand it right, SERVICE_KEY_ENV_VAR looks like the constant that stores AICORE_SERVICE_KEY as the value.

I suggested above to change those constant names in the future. At least to something like ENV_VAR_AICORE_SERVICE_KEY (if we want to avoid typing strings everywhere)

ZhongpinWang · 2026-08-13T11:28:33Z

+    fields are extracted using the ``vcap_key`` paths already defined on each ``CredentialsValue``,
+    but with the leading ``'credentials'`` segment stripped (same as the CLI's load_service_key).
+    """
+    raw = os.environ.get(SERVICE_KEY_ENV_VAR)


[req] raw is a bad name and not speaking. Consider calling it service_key_json_string.

ZhongpinWang · 2026-08-13T11:31:48Z

+def _parse_service_key(credential_values: List[CredentialsValue]) -> Optional[Callable[[CredentialsValue], Optional[str]]]:
+    """Return a source getter for AICORE_SERVICE_KEY if the env var is set and valid JSON, else None.
+
+    AICORE_SERVICE_KEY is expected to be the raw JSON string of a BTP service key, i.e. the


[pp] Not entirely. Service key and the credentials object from the VCAP variable are still... let's say not entirely the same. It is just people tend to provide the similar naming. But different services have different VCAP naming convention. The key might not even be called as credentials. Better use the word transform or map.

ZhongpinWang · 2026-08-13T11:33:57Z

 [tool.pytest.ini_options]
 testpaths = ["tests"]
 norecursedirs = ["integration_tests"]
+# Prevent pytest-dotenv from loading the repo-root .env (which contains real credentials)


[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

yamaceay requested a review from alpkom as a code owner August 12, 2026 15:08

yamaceay added 2 commits August 12, 2026 18:12

feat(core): support AICORE_SERVICE_KEY env var for credentials

cbc2752

fix: uv lock updated

0271d5b

yamaceay force-pushed the feat/aicore-service-key branch from 2b882f6 to 0271d5b Compare August 12, 2026 15:13

docs: update pydoc3 documentation [skip ci]

0de2494

marikaner reviewed Aug 13, 2026

View reviewed changes

mwien reviewed Aug 13, 2026

View reviewed changes

ZhongpinWang requested changes Aug 13, 2026

View reviewed changes

	with patch.dict(os.environ, {SERVICE_KEY_ENV_VAR: json.dumps(service_key)}):
	with patch.dict(os.environ, {AICORE_SERVICE_KEY: json.dumps(service_key)}):

		f'{AI_CORE_PREFIX}_AUTH_URL': 'https://env-auth-url',
		f'{AI_CORE_PREFIX}_BASE_URL': 'https://env-base-url',

		return str(value) if value else None


		def _parse_service_key(credential_values: List[CredentialsValue]) -> Optional[Callable[[CredentialsValue], Optional[str]]]:

-        # vcap_key is e.g. ('credentials', 'clientid') — drop the 'credentials' prefix
+        # `vcap_key` is a tuple representing the access path to properties such as
+        # `clientid` and `clientsecret` in the `aicore` service binding, e.g.
+        # `('credentials', 'clientid')`. Skip the leading path element `credentials` to
+        # access the nested value in the JSON object.

Uh oh!

Conversation

yamaceay commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marikaner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhongpinWang Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhongpinWang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhongpinWang Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yamaceay commented Aug 12, 2026 •

edited

Loading

ZhongpinWang Aug 13, 2026 •

edited

Loading

ZhongpinWang Aug 13, 2026 •

edited

Loading