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 toBuilder for resource definitions #1992

Conversation

ronjaquensel
Copy link
Contributor

What this PR changes/adds

It adds a toBuilder() method for all ResourceDefinitions.

Why it does that

During policy evaluation, functions may modify a ResourceDefinition. When each definition is re-created from scratch, information may get lost. Therefore, definitions should be convertable to a builder that can be easily modified.

Linked Issue(s)

Closes #1717

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • assigned appropriate label? (exclude from changelog with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@ronjaquensel ronjaquensel added the enhancement New feature or request label Sep 21, 2022
@ronjaquensel ronjaquensel added this to the Milestone 7 milestone Sep 21, 2022
@ronjaquensel ronjaquensel temporarily deployed to Azure-dev September 21, 2022 09:21 Inactive
@ronjaquensel ronjaquensel changed the title feature: add toBuilder for resource definitions feat: add toBuilder for resource definitions Sep 21, 2022
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.

Currently, all subclasses of ResourceDefinition have to call initialize the base class's id and transferprocessId properties by invoking

    .id(id)
    .transferProcess(transferProcessId)
    //

Maybe that could be done by the ResourceDefinition class to avoid code duplication? Also, if the base class ever receives another property, that would have t be re-done in all the subclasses...

@ronjaquensel ronjaquensel temporarily deployed to Azure-dev September 22, 2022 10:57 Inactive
@ronjaquensel
Copy link
Contributor Author

Currently, all subclasses of ResourceDefinition have to call initialize the base class's id and transferprocessId properties by invoking

    .id(id)
    .transferProcess(transferProcessId)
    //

Maybe that could be done by the ResourceDefinition class to avoid code duplication? Also, if the base class ever receives another property, that would have t be re-done in all the subclasses...

@paullatzelsperger good point, updated it

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Base: 63.63% // Head: 63.64% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (d8ddaab) compared to base (1daf73d).
Patch coverage: 76.92% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1992      +/-   ##
==========================================
+ Coverage   63.63%   63.64%   +0.01%     
==========================================
  Files         772      772              
  Lines       16357    16370      +13     
  Branches     1055     1055              
==========================================
+ Hits        10408    10418      +10     
- Misses       5505     5508       +3     
  Partials      444      444              
Impacted Files Coverage Δ
.../spi/types/domain/transfer/ResourceDefinition.java 84.21% <0.00%> (-15.79%) ⬇️
...on/azure/blob/ObjectStorageResourceDefinition.java 100.00% <100.00%> (ø)
...sion/http/impl/AbstractHttpResourceDefinition.java 100.00% <100.00%> (ø)
...sion/http/impl/HttpProviderResourceDefinition.java 100.00% <100.00%> (ø)
...r/aws/s3/provision/S3BucketResourceDefinition.java 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ronjaquensel ronjaquensel temporarily deployed to Azure-dev September 23, 2022 08:15 Inactive
@ronjaquensel ronjaquensel force-pushed the feature/1717-resource-definition-to-builder branch from 1efc0d6 to 6e0202a Compare September 23, 2022 08:46
@ronjaquensel ronjaquensel temporarily deployed to Azure-dev September 23, 2022 08:47 Inactive
@ronjaquensel ronjaquensel temporarily deployed to Azure-dev September 23, 2022 08:52 Inactive
@ronjaquensel ronjaquensel merged commit 740c100 into eclipse-edc:main Sep 23, 2022
@ronjaquensel ronjaquensel deleted the feature/1717-resource-definition-to-builder branch September 23, 2022 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toBuilder method for resource definitions
3 participants