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: remove ConnectorVersionProvider #1470

Conversation

juliapampus
Copy link
Contributor

@juliapampus juliapampus commented Jun 13, 2022

What this PR changes/adds

Renames and moves ConnectorVersionProvider interface and impl from ids spi to core spi.

Why it does that

Resolve TODO. The software version is not an IDS specific property.

Further notes

Is spi.system.version the right location, or is there a better one?

Linked Issue(s)

Closes #1466

Checklist

  • performed checkstyle check locally?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@juliapampus juliapampus changed the title refactor(ConnectorVersionProvider): rename to ConnectorVersion and move from ids to general spi refactor: rename to ConnectorVersion and move from ids to general spi Jun 13, 2022
@juliapampus juliapampus changed the title refactor: rename to ConnectorVersion and move from ids to general spi refactor: rename ConnectorVersion and move from ids to general spi Jun 13, 2022
@juliapampus juliapampus force-pushed the refactor/1466_move_connector_version branch from c6f4522 to 8412fbf Compare June 13, 2022 11:43
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2022

Codecov Report

Merging #1470 (2c9ada1) into main (0671bb5) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##             main    #1470   +/-   ##
=======================================
  Coverage   67.16%   67.16%           
=======================================
  Files         717      716    -1     
  Lines       15978    15972    -6     
  Branches     1057     1057           
=======================================
- Hits        10731    10728    -3     
+ Misses       4773     4770    -3     
  Partials      474      474           
Impacted Files Coverage Δ
...aceconnector/ids/core/IdsCoreServiceExtension.java 0.00% <0.00%> (ø)
...nnector/ids/core/service/ConnectorServiceImpl.java 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0671bb5...2c9ada1. Read the comment docs.

@jimmarino
Copy link
Contributor

Can we rename this to EdcVersion since this can be used by all EDC runtimes and not just the connector?

@juliapampus
Copy link
Contributor Author

Can we rename this to EdcVersion since this can be used by all EDC runtimes and not just the connector?

Sure

@juliapampus juliapampus force-pushed the refactor/1466_move_connector_version branch from 8412fbf to 1a67b35 Compare June 14, 2022 06:34
@juliapampus juliapampus requested a review from jimmarino June 14, 2022 07:30

// TODO move to edc-spi
public interface ConnectorVersionProvider {
public interface EdcVersion {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need all these interfaces and impls for a simple version string?

How about we let the ConnectorServiceImpl either use a hard-coded version string (0.0.1-SNAPSHOT) instead of the ConnectorVersionProvider, and we delete the ConnectorVersionProvider and the EdcVersion?
In the future we could improve this:

  • create a gradle task that updates this hard-coded value before compile/build
  • create a gradle task that generates a separate version file during build, which is then parsed by the ConnectorServiceImpl. That file would have to be shipped together with EDC code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paullatzelsperger Changed as proposed

@juliapampus juliapampus force-pushed the refactor/1466_move_connector_version branch from 1a67b35 to 56ddb00 Compare June 15, 2022 05:44
@juliapampus juliapampus changed the title refactor: rename ConnectorVersion and move from ids to general spi refactor: remove ConnectorVersionProvider Jun 15, 2022
@juliapampus juliapampus force-pushed the refactor/1466_move_connector_version branch from 56ddb00 to 12e621f Compare June 15, 2022 05:48
@juliapampus juliapampus force-pushed the refactor/1466_move_connector_version branch from 12e621f to 44469b4 Compare June 15, 2022 05:51
@juliapampus juliapampus force-pushed the refactor/1466_move_connector_version branch from 44469b4 to 2c9ada1 Compare June 15, 2022 06:23
Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

LGTM

@juliapampus juliapampus merged commit 50c941a into eclipse-edc:main Jun 15, 2022
@juliapampus juliapampus deleted the refactor/1466_move_connector_version branch June 15, 2022 06:51
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.

Move ConnectorVersion interface from ids to edc core
5 participants