Skip to content

Conversation

adityajoshi12
Copy link

No description provided.

Signed-off-by: Aditya Joshi <[email protected]>
@SamYuan1990
Copy link
Contributor

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
Copy link
Author

Hi @denyeart , can you review this PR?

@bestbeforetoday
Copy link
Member

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
Copy link
Author

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
Copy link
Member

bestbeforetoday commented May 14, 2024

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
Copy link
Author

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
Copy link
Member

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.

Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I like the intent of the PR to extend the admin API capability. I suggest changes as described in the previous comments.

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.

3 participants