search

hyperledger / fabric-admin-sdk

Fabric SDK for Admin Capability services
Apache License 2.0
31 stars 19 forks source link

moved osn package #196

Open adityajoshi12 opened 2 months ago

SamYuan1990 commented 2 months ago

As a personal suggestion, if @denyeart/ @bestbeforetoday can offline review this PR that would be fine. otherwise, @aldousalvarez , you'd better join fabric contributor meeting or other approach to contact with Dave or Mark.

adityajoshi12 commented 1 month ago

Hi @denyeart , can you review this PR?

bestbeforetoday commented 1 month ago

I'm sure there was a discussion somewhere else but I don't find it linked to this PR.

The osnadmin package that this PR exposes in the public API is pretty low-level. I'm not sure if *http.Response is really the ideal return type for users of fabric-admin-sdk. Much of the newly exposed functionality also duplicates (slightly) higher level capability already exposed by the channel package.

What exactly are we trying to achieve with this PR?

adityajoshi12 commented 1 month ago

I'm sure there was a discussion somewhere else but I don't find it linked to this PR.

The osnadmin package that this PR exposes in the public API is pretty low-level. I'm not sure if *http.Response is really the ideal return type for users of fabric-admin-sdk. Much of the newly exposed functionality also duplicates (slightly) higher level capability already exposed by the channel package.

What exactly are we trying to achieve with this PR?

Hi @denyeart Currently, fabric-admin-sdk doesn't exposes the public API to deal with channel operations related to orderer, like join, remove, etc. The changeset in this PR make the osn package public so that it can be consumed in downstream application, similar to hlf-connector which uses fabric-sdk-java to deal with such operations

bestbeforetoday commented 1 month ago

I see a channel.CreateChannel function, which does the same thing as osnadmin.Join. There is also a channel.ListChannel function, which does the same thing as osnadmin.ListAllChannels. The functions in the channel package seem to offer a (slightly) more friendly API for application code, doing some marshalling of input parameters and unpacking HTTP responses. Perhaps we just could refine and extend the approach of the channel package instead of expose the low-level osnadmin package?

adityajoshi12 commented 1 month ago

I see a channel.CreateChannel function, which does the same thing as osnadmin.Join. There is also a channel.ListChannel function, which does the same thing as osnadmin.ListAllChannels. The functions in the channel package seem to offer a (slightly) more friendly API for application code, doing some marshalling of input parameters and unpacking HTTP responses. Perhaps we just could refine and extend the approach of the channel package instead of expose the low-level osnadmin package?

Thanks for the reference, however it still misses few functionality like, orderer removal, also would be nice if we have high level API that adds the orderer to the consenter list.

bestbeforetoday commented 1 month ago

Can we extend the channel package with the required capability, exposed in a more application-friendly manner than osnadmin? Not just turn osnadmin into public API.

It's a good opportunity to see if what's there can be improved too. At first glance, having *http.Response as a return value doesn't look ideal. Maybe some different naming would improve clarity too.