Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make CredentialService URL configurable #3635

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Nov 21, 2023

What this PR changes/adds

This PR adds a configuration value for the URL of the credentialservice, which is a mandatory config parameter
when using IATP

Why it does that

Being able to forward credential requests

Further notes

  • This PR also moves the injection of the DefaultCredentialServiceClient to the defaults extension, to make it easily replaceable
  • for an embedded CredentialServiceClient one would simply have to provide another implementation analogous to the DefaultCredentialServiceClient

Linked Issue(s)

Closes # <-- insert Issue number if one exists

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@paullatzelsperger paullatzelsperger force-pushed the feat/iatp_credentialservice_url_configurable branch from c84cda7 to fe0769e Compare November 21, 2023 06:44
@codecov-commenter
Copy link

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (c609610) 71.77% compared to head (fe0769e) 71.76%.

Files Patch % Lines
...entitytrust/core/IatpDefaultServicesExtension.java 0.00% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3635      +/-   ##
==========================================
- Coverage   71.77%   71.76%   -0.01%     
==========================================
  Files         914      914              
  Lines       18276    18278       +2     
  Branches     1036     1036              
==========================================
+ Hits        13117    13118       +1     
- Misses       4707     4708       +1     
  Partials      452      452              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paullatzelsperger paullatzelsperger merged commit 36ab86d into eclipse-edc:main Nov 21, 2023
19 checks passed
public CredentialServiceClient createClient(ServiceExtensionContext context) {
return new DefaultCredentialServiceClient(httpClient, Json.createBuilderFactory(Map.of()),
typeManager.getMapper(JSON_LD), typeTransformerRegistry, jsonLd, context.getMonitor(),
context.getConfig().getString(CREDENTIALSERVICE_URL_PROPERTY));
Copy link
Contributor

Choose a reason for hiding this comment

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

@paullatzelsperger are we sure jsonLd will be injected before this method is invoked? (as it is a isDefault = true)

Copy link
Member Author

Choose a reason for hiding this comment

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

mmh, interesting point. Theoretically, it could happen, if some extension injects the CredentialServiceClient before the JsonLd service gets created. The safe way would probably be to remove the isDefault=true attribute, which also makes replacing the Default...Client harder to replace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request iatp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants