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: Add capability to register an embedded dataplane #3902

Closed

Conversation

zub4t
Copy link
Contributor

@zub4t zub4t commented Feb 21, 2024

What this PR changes/adds

This PR contributes with an extension to enable an embedded data plane to be register using a configuration file without the need to do explicitly through an API

Why it does that

When doing unit tests and there is a necessity to register an embedded data plane, before it was necessary to do so through an API request, now it is possible to use configuration.

Linked Issue(s)

Closes #2226

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.

@zub4t zub4t force-pushed the feature/embedded-data-plane-registration branch from 05f5b59 to 2bcdb1b Compare February 21, 2024 16:40
@zub4t zub4t force-pushed the feature/embedded-data-plane-registration branch from 2bcdb1b to 4da6f51 Compare February 21, 2024 16:50
Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I have some serious concerns as this code appears to be a near copy of this code without attribution:

https://github.com/eclipse-tractusx/tractusx-edc/blob/main/edc-extensions/dataplane-selector-configuration/src/main/java/org/eclipse/tractusx/edc/dataplane/selector/configuration/DataPlaneSelectorConfigurationServiceExtension.java

We can't have that. Also:

  1. The code style does not adhere to EDC guidelines
  2. There is no documentation
  3. The implementation can be simplified and clarified in places

My suggestion is to close this PR due to the issue of improper attribution, open a discussion, and let's work through how to approach the implementation from a clean slate.

@zub4t
Copy link
Contributor Author

zub4t commented Feb 21, 2024

Ok, sure. Let's do it 😊

@jimmarino jimmarino closed this Feb 22, 2024
@zub4t
Copy link
Contributor Author

zub4t commented Feb 22, 2024

Hi @jimmarino Thanks for your review 😊
Can you help me understand points 1 and 3 better? I'd like to improve my code for future PR

@jimmarino
Copy link
Contributor

Please review the coding guidelines (including running checkstyle). For point 3, open a discussion with a technical proposal for how you want the registration process to work. I want to start from a clean slate, not the current code.

@zub4t
Copy link
Contributor Author

zub4t commented Feb 23, 2024

Please review the coding guidelines (including running checkstyle). For point 3, open a discussion with a technical proposal for how you want the registration process to work. I want to start from a clean slate, not the current code.

Okay, thanks again, I'll do as you suggest

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.

Register embedded data-plane for sync transfer by default
2 participants