Skip to content

feat(core): support AICORE_SERVICE_KEY env var for credentials and rename env vars - #58

Open
yamaceay wants to merge 9 commits into
mainfrom
feat/aicore-service-key
Open

feat(core): support AICORE_SERVICE_KEY env var for credentials and rename env vars#58
yamaceay wants to merge 9 commits into
mainfrom
feat/aicore-service-key

Conversation

@yamaceay

@yamaceay yamaceay commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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.

@yamaceay
yamaceay requested a review from alpkom as a code owner August 12, 2026 15:08
@yamaceay
yamaceay force-pushed the feat/aicore-service-key branch from 2b882f6 to 0271d5b Compare August 12, 2026 15:13

@marikaner marikaner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread packages/core/ai_core_sdk/helpers/constants.py Outdated
)
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread packages/core/tests/ai_core_client/test_credentials.py Outdated
Comment thread packages/core/tests/ai_core_client/test_credentials.py
Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/pyproject.toml Outdated
Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/ai_core_sdk/credentials.py Outdated

@ZhongpinWang ZhongpinWang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

First time reviewing python SDK :)

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

Comment thread packages/core/ai_core_sdk/helpers/constants.py Outdated
Comment thread packages/core/ai_core_sdk/helpers/constants.py Outdated
) from exc

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also got super confused by this, but I also believe that this might be because I am not used to python as much.

Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/docs/ai_core_sdk.helpers.constants.html
Comment thread packages/core/tests/ai_core_client/test_credentials.py Outdated
Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/pyproject.toml Outdated
[tool.pytest.ini_options]
testpaths = ["tests"]
norecursedirs = ["integration_tests"]
# Prevent pytest-dotenv from loading the repo-root .env (which contains real credentials)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am just not entirely sure if we should or must put .env at root. Not sure what python colleagues previously used to do.

@yamaceay yamaceay Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 1

@ZhongpinWang ZhongpinWang Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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?

@yamaceay yamaceay changed the title feat(core): support AICORE_SERVICE_KEY env var for credentials feat(core): support AICORE_SERVICE_KEY env var for credentials and rename env vars Aug 14, 2026
@yamaceay
yamaceay force-pushed the feat/aicore-service-key branch from 90d6eb6 to 6861739 Compare August 14, 2026 08:54
@yamaceay
yamaceay requested a review from ZhongpinWang August 14, 2026 09:09
Comment thread packages/core/ai_core_sdk/credentials.py Outdated
def _get_nested_safe(data: Dict, keys) -> Optional[Any]:
try:
return get_nested_value(data, keys)
except KeyError:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pp] Maybe add a warning log here? I in general don't feel super comfortable to silent some exceptions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Comment thread packages/core/ai_core_sdk/credentials.py Outdated
Comment thread packages/core/ai_core_sdk/credentials.py Outdated

@ZhongpinWang ZhongpinWang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks much better now! Thanks for your effort 👍

@ZhongpinWang
ZhongpinWang dismissed their stale review August 14, 2026 09:34

Changes applied.

Co-authored-by: Zhongpin Wang <zhongpin.wang@sap.com>
@yamaceay
yamaceay force-pushed the feat/aicore-service-key branch from 099e8da to 4d72a82 Compare August 14, 2026 10:59
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.

4 participants