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(api): counterPartyAddress #3597

Merged

Conversation

saschaisele-zf
Copy link
Contributor

@saschaisele-zf saschaisele-zf commented Nov 10, 2023

What this PR changes/adds

Deprecates the remaining uses of connectorAddress and providerUrl in the ContractNegotiationApi and TransferProcessApi.
Instead, counterPartyAddress is used going forward.

Why it does that

Use of consistent naming across the management api.

Further notes

It is important to note that this is a breaking change.
CAUTION ADVICED WHEN MERGING.

Linked Issue(s)

Closes #3343

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

@saschaisele-zf saschaisele-zf changed the title refactor(management-api): counterPartyAddress (#3343) refactor(management-api): counterPartyAddress Nov 10, 2023
@saschaisele-zf saschaisele-zf marked this pull request as draft November 10, 2023 14:54
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 also note that this is a breaking change because it is not backwards compatible with prior connector versions before this commit?

Note, we should also discuss when to merge this.

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023

Codecov Report

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

Comparison is base (0d6086d) 72.09% compared to head (4407f12) 72.10%.

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3597   +/-   ##
=======================================
  Coverage   72.09%   72.10%           
=======================================
  Files         897      897           
  Lines       17920    17949   +29     
  Branches     1018     1024    +6     
=======================================
+ Hits        12920    12942   +22     
- Misses       4563     4568    +5     
- Partials      437      439    +2     
Files Coverage Δ
...r/transfer/process/TransferProcessManagerImpl.java 94.96% <100.00%> (ø)
...nt/catalog/validation/CatalogRequestValidator.java 93.75% <100.00%> (+0.89%) ⬆️
...nt/contractnegotiation/ContractNegotiationApi.java 0.00% <ø> (ø)
...gotiation/validation/ContractRequestValidator.java 95.00% <100.00%> (+5.00%) ⬆️
...management/transferprocess/TransferProcessApi.java 0.00% <ø> (ø)
...t/transferprocess/TransferProcessApiExtension.java 91.66% <100.00%> (ø)
...ontract/spi/types/negotiation/ContractRequest.java 0.00% <ø> (ø)
...nsform/JsonObjectToContractRequestTransformer.java 96.00% <75.00%> (-4.00%) ⬇️
...erprocess/validation/TransferRequestValidator.java 85.71% <81.81%> (-5.20%) ⬇️
.../connector/transfer/spi/types/TransferRequest.java 0.00% <0.00%> (ø)
... and 1 more

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

@jimmarino jimmarino requested a review from ndr-brt November 10, 2023 15:05
@jimmarino
Copy link
Contributor

Added a note in the review: we need to discuss when to introduce this change due to DSP backward-compatibility issues.

Signed-off-by: Sascha Isele <[email protected]>
@saschaisele-zf saschaisele-zf changed the title refactor(management-api): counterPartyAddress refactor(api): counterPartyAddress Nov 10, 2023
@saschaisele-zf saschaisele-zf marked this pull request as ready for review November 10, 2023 16:29
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.

just minor comments about the warning messages.
@jimmarino I think this PR is not a breaking change (everything will work in the same way), the break part of the change will happen when we'll remove the old attributes. I don't see change in the DSP protocol either

@ndr-brt ndr-brt added refactoring Cleaning up code and dependencies api Feature related to the (REST) api labels Nov 13, 2023
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

@ndr-brt ndr-brt merged commit dd99058 into eclipse-edc:main Nov 16, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Feature related to the (REST) api refactoring Cleaning up code and dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rename providerUrl and connectorAddress to counterPartyAddress on management api
4 participants