-
Notifications
You must be signed in to change notification settings - Fork 2
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: update asset endpoints #36
Feat: update asset endpoints #36
Conversation
I tagged @fdionisi as the main reviewer to validate this PR from an approach and coding perspective. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something we should do to enable testing?
Have we considered already using some json-ld
package to avoid working on the types ourselves?
@fdionisi thank you so much for taking the time to review this.
|
@OlfaBensoussia, sorry for my poor communication. I meant that we should make tests pass, but now we are on the latest version of the connector. We should try land #35 first and then use the docker image to endure correct functionality on this PR. I don't think we'll be developing anymore for milestone-8 (except for #20, for now). Hence, it should be find to proceed |
I hear you. It makes sense to me for now, but I would like to explore the possibility of using a json-ld-aware library to perform input validation - if the format is not correct, then it's not worth bothering the backend in my opinion. Let's keep this exploration for later. |
4e35f26
to
bfe2221
Compare
@ndr-brt I tried to fix most of the failing tests, remove any hardcoded values and update most of the entities. As far as it concerns the management tests, they are all now passing except the ones that rely on the function |
I will take a look at it, great work so far. 🙌 |
@OlfaBensoussia I fixed stuff here and there, the CI is green. I'll propose to merge this into my PR and then I will fixup some cosmetics that needs to be in place before release, let me know. EDIT: it works on my computer but not on github 😆 |
b5a85c5
to
280203b
Compare
02207ee
to
17d2b14
Compare
17d2b14
to
f830445
Compare
@ndr-brt Thank you for the clarifications. I will merge this and we can fix the rest of the tests on the original branch |
* build(deps): upgrade EDC to milestone-9 * Upgrade connector to 0.1.0 * Fix public.test.ts * Feat: update asset endpoints (#36) * chore: update old asset entities specs & add new context * chore: use new connector entities in management controllers and utils * test: adjust asset tests * chore: update entities * fix: fix contract, policy and asset failing tests * chore: update addresses props in connector config files * refactor: Standardize request body objects and update entities * fix: update catalog and contract negotiations tests * Fix CI --------- Co-authored-by: ndr_brt <[email protected]> * refactor: remove ternary operator * add missing providerId field * Print logs * add logs * PR remarks * Refactor --------- Co-authored-by: Olfa <[email protected]> Co-authored-by: OlfaBensoussia <[email protected]>
Description
With the EDC milestone 9 release, there were some changes when it comes to the asset related entities.
This PR addresses all of them but doesn't add the new asset endpoints.
To sum it up, we:
How to test it
Approach
{{ConnectorManagementURL}}/v2/assets
{{ConnectorManagementURL}}/v2/assets/request
{{ConnectorManagementURL}}/v2/assets/{{AssetId}}
{{ConnectorManagementURL}}/v2/assets/{{AssetId}}/dataaddress
Open Questions and Pre-Merge TODOs
description
,name
andcontenttype
. I remember we have more than that but I will need to go through the base code to get them.