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

Add capabilities during org addition to channel #140

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

DeepikaKaranji
Copy link
Contributor

Add capabilities during org addition to channel

Signed-off-by: Deepika Karanji - d0k03k3 <[email protected]>
@DeepikaKaranji DeepikaKaranji marked this pull request as ready for review March 20, 2024 04:55
@DeepikaKaranji DeepikaKaranji requested a review from a team as a code owner March 20, 2024 04:55
Comment on lines 229 to 245
/**
* @param capabilities capabilities need to be added to config
* @return channel capabilities
*/
private Configtx.ConfigValue getCapabilities(String... capabilities) {
Configtx.ConfigValue.Builder valueBuilder = Configtx.ConfigValue.newBuilder();
Configuration.Capabilities.Builder capabilitiesBuilder =
Configuration.Capabilities.newBuilder();
valueBuilder.setModPolicy(DEFAULT_MOD_POLICY);
valueBuilder.setVersion(0);
for (String capability : capabilities) {
capabilitiesBuilder.putCapabilities(
capability, Configuration.Capability.newBuilder().build());
}
valueBuilder.setValue(capabilitiesBuilder.build().toByteString());
return valueBuilder.build();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

FabricChannelUtil is there for all the channel related code that is needed for channel creation and update, move this code and ChannelServiceImpl in that class and call that method in this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I note your point regarding moving thi function to FabricChannelUtil, but since the ChannelService class exists, its impl class should also exist and not be moved to a util in the context of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the duplicated code wrt getCapabilities in ChannelServiceImpl and your PR code, move it to FabricChannelUtil

Comment on lines 30 to 34
private static final String VALUE_TAG_CAPABILITIES = "Capabilities";

private static final String FABRIC_2_0 = "V2_0";

private static final String DEFAULT_MOD_POLICY = "Admins";
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's define the constants in FabricClientConstants and re-use. I see these constants being defined in other places like ChannelServiceImpl, could be picked from one place.

Comment on lines 402 to 404
System.out.println(user.getName());
System.out.println(user.getRoles());
System.out.println(user.getEnrollment());
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these. If there is a valid reason to display these in the runtime trace, log it with DEBUG level

* @param capabilities capabilities need to be added to config
* @return channel capabilities
*/
public Configtx.ConfigValue getCapabilities(String... capabilities) {
Copy link
Contributor

@nithin-pankaj nithin-pankaj Mar 21, 2024

Choose a reason for hiding this comment

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

[nitpick] getCapabilitiesWithDefaultVersion

@DeepikaKaranji DeepikaKaranji force-pushed the main branch 2 times, most recently from 65bcbb5 to 096e467 Compare March 21, 2024 08:55
Signed-off-by: Deepika Karanji - d0k03k3 <[email protected]>
@nithin-pankaj nithin-pankaj merged commit 0019998 into hyperledger-labs:main Mar 21, 2024
4 checks passed
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.

4 participants