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

chore: deprecate provider_id field in ContractRequest #3948

Conversation

Pluci
Copy link
Contributor

@Pluci Pluci commented Mar 4, 2024

What this PR changes/adds

Deprecated provider_id field in ContractRequest

Why it does that

Clean up.

Linked Issue(s)

Closes #3929

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

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.

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.

hey @Pluci I think the objective here would be not just to deprecate a constant, but also update the api spec (currently that field is mandatory on the management api) and also remove the providerId field from the ContractRequest POJO and ensure that the information is extracted from the enclosed policy

@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 18.75000% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 65.42%. Comparing base (7f20ba5) to head (ef69102).
Report is 143 commits behind head on main.

Files Patch % Lines
...ator/jsonobject/validators/LogDeprecatedValue.java 0.00% 12 Missing ⚠️
...ontract/spi/types/negotiation/ContractRequest.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    #3948      +/-   ##
==========================================
- Coverage   71.74%   65.42%   -6.32%     
==========================================
  Files         919      976      +57     
  Lines       18457    19706    +1249     
  Branches     1037     1107      +70     
==========================================
- Hits        13242    12893     -349     
- Misses       4756     6377    +1621     
+ Partials      459      436      -23     

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

@ndr-brt ndr-brt added enhancement New feature or request refactoring Cleaning up code and dependencies api Feature related to the (REST) api labels Mar 4, 2024
@Pluci
Copy link
Contributor Author

Pluci commented Mar 4, 2024

hey @Pluci I think the objective here would be not just to deprecate a constant, but also update the api spec (currently that field is mandatory on the management api) and also remove the providerId field from the ContractRequest POJO and ensure that the information is extracted from the enclosed policy

hi @ndr-brt, thank you for the suggestions. I'm going to fix those as well.

@Pluci Pluci requested a review from paullatzelsperger as a code owner March 4, 2024 13:47
@Pluci Pluci requested a review from ndr-brt March 7, 2024 17:43
@Pluci Pluci requested a review from ndr-brt March 11, 2024 08:30
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.

already mergeable, I will wait for the validator refactor and the dependencies file fix

@Pluci Pluci requested a review from ndr-brt March 12, 2024 17:28
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.

2 nits, please fix checkstyle issues and dependecies file and we'll be ready to go 👍

@Pluci Pluci requested a review from ndr-brt March 13, 2024 10:43
@Pluci
Copy link
Contributor Author

Pluci commented Mar 13, 2024

The failing unit test is working fine in the local environment, so I have no idea why it's failing on the CI/CD pipeline.

@ndr-brt
Copy link
Member

ndr-brt commented Mar 13, 2024

The failing unit test is working fine in the local environment, so I have no idea why it's failing on the CI/CD pipeline.

sincerely I got no clue, also on my machine it works correctly, anyway, please try to change line 80 of that test this way:
.satisfies(transformed -> assertThat(transformed.getProtocol()).isNotBlank()));

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 a8e69fd into eclipse-edc:main Mar 14, 2024
17 checks passed
@Pluci Pluci deleted the issues/3929/ContractRequest.providerId-is-not-needed-anymore-and-can-be-deprecated branch March 17, 2024 13:30
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 enhancement New feature or request refactoring Cleaning up code and dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ContractRequest.providerId is not needed anymore and can be deprecated
3 participants