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

refactor: split data plane module into two separate modules #3676

Conversation

hamidonos
Copy link
Contributor

@hamidonos hamidonos commented Nov 30, 2023

What this PR changes/adds

  • Split data-plane-api module into two separate modules data-plane-control-api & data-plane-public-api
  • Map data-plane-public-api module to public-api group

Why it does that

Interfaces under data-plane-public-api should not be under control-api api group because this api group is intended for internal communication.

Further notes

This introduces a breaking change because the data-plane-api module has been split into data-plane-control-api and data-plane-public-api. Downstream projects need to adjust their dependencies.

Note that the migration of the data-plane-api README file will be handled in a separate PR.

Linked Issue(s)

Closes #3373
Depends on #3686

@hamidonos hamidonos self-assigned this Nov 30, 2023
@hamidonos hamidonos added documentation Improvements or additions to documentation refactoring Cleaning up code and dependencies labels Nov 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2023

Codecov Report

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

Comparison is base (bdd6a2a) 71.73% compared to head (b1da7d9) 71.72%.

Files Patch % Lines
...or/dataplane/api/DataPlaneControlApiExtension.java 0.00% 4 Missing ⚠️
.../api/controller/DataPlaneControlApiController.java 66.66% 2 Missing ⚠️
...tor/dataplane/api/DataPlanePublicApiExtension.java 0.00% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3676      +/-   ##
==========================================
- Coverage   71.73%   71.72%   -0.02%     
==========================================
  Files         919      919              
  Lines       18465    18468       +3     
  Branches     1034     1034              
==========================================
  Hits        13246    13246              
- Misses       4760     4763       +3     
  Partials      459      459              

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

@hamidonos hamidonos marked this pull request as draft November 30, 2023 14:03
@hamidonos hamidonos marked this pull request as ready for review November 30, 2023 15:20
@hamidonos hamidonos added the breaking-change Will require manual intervention for version update label Nov 30, 2023
.github/workflows/apidoc.yaml Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-control-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-control-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-control-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-control-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-public-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-public-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-public-api/README.md Outdated Show resolved Hide resolved
extensions/data-plane/data-plane-public-api/README.md Outdated Show resolved Hide resolved
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.

Can we please break this into two PRs because the module shift has implications for downstream repositories and is not a "doc" change?

@hamidonos
Copy link
Contributor Author

Can we please break this into two PRs because the module shift has implications for downstream repositories and is not a "doc" change?

The creation of the new public hub & split of the modules is the minimum requirement for this PR
If we split the "doc" part into a separate PR then we will release two modules (this PR) with wrong/missing doc
To me the modularization + doc change is one unit

@jimmarino
Copy link
Contributor

Can we please break this into two PRs because the module shift has implications for downstream repositories and is not a "doc" change?

The creation of the new public hub & split of the modules is the minimum requirement for this PR If we split the "doc" part into a separate PR then we will release two modules (this PR) with wrong/missing doc To me the modularization + doc change is one unit

We are not releasing the doc at this point. The two changes are distinct, particularly because the module split has significant downstream implications. Please make these changes in two separate PRs so that they can be reviewed and documented correctly.

…into_dedicated_swagger_hub_page' into docs/publish_public_api_context_into_dedicated_swagger_hub_page

# Conflicts:
#	extensions/data-plane/data-plane-control-api/README.md
#	extensions/data-plane/data-plane-public-api/README.md
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

LGTM

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.

This PR does not appear to break out the changes into two PRs as requested (at least the title does not indicate that, and my issue was not marked as resolved).

Please put the module refactor in its own PR.

@hamidonos hamidonos changed the title docs: publish public api context into dedicated swagger hub page enhancement: publish public api context into dedicated swagger hub page Dec 4, 2023
@hamidonos hamidonos added enhancement New feature or request and removed documentation Improvements or additions to documentation labels Dec 4, 2023
@hamidonos hamidonos changed the title enhancement: publish public api context into dedicated swagger hub page refactor: split data plane module into two separate modules Dec 5, 2023
@hamidonos hamidonos marked this pull request as draft December 5, 2023 09:38
@hamidonos hamidonos removed the enhancement New feature or request label Dec 5, 2023
…_api_context_into_dedicated_swagger_hub_page
@hamidonos hamidonos marked this pull request as ready for review December 5, 2023 10:08
@hamidonos
Copy link
Contributor Author

This PR does not appear to break out the changes into two PRs as requested (at least the title does not indicate that, and my issue was not marked as resolved).

Please put the module refactor in its own PR.

This PR is now solely responsible for the module migration

I'm unable to resolve your comment tho

@ndr-brt ndr-brt merged commit dba3109 into eclipse-edc:main Dec 11, 2023
17 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Will require manual intervention for version update refactoring Cleaning up code and dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish public api context into dedicated swagger hub page
5 participants