Skip to content

Conversation

@newtork
Copy link
Contributor

@newtork newtork commented Oct 27, 2025

Context

Related:
https://github.com/SAP/ai-sdk-java-backlog/issues/295

Feature scope:

  • Add caching
    • Reduce IO
    • Reduce logging to evaluate log-level

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Aligned changes with the JavaScript SDK
  • Documentation updated
  • Release notes updated

@CharlesDuboisSAP CharlesDuboisSAP mentioned this pull request Oct 27, 2025
10 tasks
@newtork newtork changed the title chore: Add 5min cache to service binding loader chore: Reuse DotEnv instance and reduce log messages Oct 28, 2025
@newtork newtork added the please-review Request to review a pull-request label Oct 28, 2025
Copy link
Member

@rpanackal rpanackal left a comment

Choose a reason for hiding this comment

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

LGTM. But there are conflicts to resolve

Comment on lines 58 to 59
final String logMessage =
"Found a service key in environment variable {}.\nUsing a service key is recommended for local testing only.\nBind the AI Core service to the application for productive usage.\n";
Copy link
Member

Choose a reason for hiding this comment

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

(preference)

Here is a shorter version.

Suggested change
final String logMessage =
"Found a service key in environment variable {}.\nUsing a service key is recommended for local testing only.\nBind the AI Core service to the application for productive usage.\n";
final String logMessage =
"Service key found in env {}. Recommended for local testing only. Prefer service binding in production.";

Copy link
Member

Choose a reason for hiding this comment

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

(Preference)

I like the longer message as it is more descriptive and (I think) clearer for the user what to do. But I would remove the newlines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the newlines, prefer the "longer" version a little more as it is very precise and has a call to action.

Copy link
Member

@Jonas-Isr Jonas-Isr left a comment

Choose a reason for hiding this comment

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

👍

@Jonas-Isr Jonas-Isr merged commit 24a77f4 into main Oct 30, 2025
7 checks passed
@Jonas-Isr Jonas-Isr deleted the cache-aicore-service-binding-loader branch October 30, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please-review Request to review a pull-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants