-
Notifications
You must be signed in to change notification settings - Fork 13
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
Marketplace service new api for manifest #837
Conversation
c1f0362
to
796311d
Compare
9d15cdf
to
00980ed
Compare
00980ed
to
8abb1c7
Compare
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.
I agree with the goal of this PR but I have some concerns about oversimplifying too much the returned data.
There is only the manifest validation that overlap on the other PR.
Otherwise I put mostly naming suggestions.
Edit:
Only returning the manifest's data but not manifestProtocol
and manifestData
is too much for my point of view. Those 2 info, even if they are completely managed by the service, are still valid to return.
As the marketplace is on Ethereum, anyone can add a new version with wrong manifest data. In this PR's current implementation, the version is discarded and it will appear that the service has no version, even if it actually has one. The wrong version cannot be deployed on a core, but should still be return as a version with wrong manifest. Othercase: the manifest is valid but for some reason is not available on IPFS anymore?
I think returning the version even if the manifest is not valid / available is better than just discard it.
16f959d
to
7f3e8e7
Compare
All the unnecessary dev on this PR has been reverted, now only the |
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.
There is a bug when uploading a service. Please fix.
Otherwise, the modif is good 👍
…ndation/core into feature/marketplace-api
6de3170
to
e2d724e
Compare
e2d724e
to
61fe9fe
Compare
The goal here is to add validation of the manifest data directly with the core by passing all arguments necessary to the service and giving responsibility to the service to manage how it wants to upload this manifest (with IPFS or whatever).
This remove a lot of logic from the client and also remove the management of version from the client, now the service manages the version (we change the service == we change the version).
The client doesn't need to know anything about how data are stored in the marketplace IPFS or no. For now we still need to upload the sources but ultimately we should even give directly the tarball to the system service and let it handle everything
@NicolasMahe want to have your feedback before I continue to make sure that this is aligned with what you want with #829 and #835
TODO:
./dev-cli service execute marketplace --task getService --data sid=myservice