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

[FAB-17466] Generate create channel transaction #686

Merged
merged 1 commit into from
Feb 20, 2020

Conversation

stephyee
Copy link
Contributor

Signed-off-by: Tiffany Harris tiffany.harris@ibm.com
Signed-off-by: Danny Cao dcao@us.ibm.com

Type of change

  • New feature

Description

Generates a channel create transaction using the a profile and msp config

Related issues

FAB-17466

@stephyee stephyee requested a review from a team as a code owner February 17, 2020 14:44
@stephyee stephyee force-pushed the fab-17466 branch 3 times, most recently from 68a6db0 to 905c272 Compare February 17, 2020 18:56
// operation fails.
func MarshalOrPanic(pb proto.Message) []byte {
func protoMarshalOrPanic(pb proto.Message) []byte {
data, err := proto.Marshal(pb)
Copy link
Contributor

Choose a reason for hiding this comment

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

we might not want to panic at all in this package?
#669 (comment)

Copy link

Choose a reason for hiding this comment

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

addressed in latest commit

Copy link
Contributor

@wlahti wlahti left a comment

Choose a reason for hiding this comment

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

A number of nits for your consideration. :)

pkg/config/application.go Outdated Show resolved Hide resolved
pkg/config/signer.go Outdated Show resolved Hide resolved
pkg/config/update.go Outdated Show resolved Hide resolved
pkg/config/policyparser.go Outdated Show resolved Hide resolved
pkg/config/policyparser.go Outdated Show resolved Hide resolved
pkg/config/policyparser.go Outdated Show resolved Hide resolved
pkg/config/update.go Outdated Show resolved Hide resolved
pkg/config/update_test.go Outdated Show resolved Hide resolved
wlahti
wlahti previously approved these changes Feb 19, 2020
Copy link
Contributor

@wlahti wlahti left a comment

Choose a reason for hiding this comment

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

Looks good overall to me! Would be nice to get these current config PRs in so we can keep moving forward and make any minor improvements along the way.

Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>
Signed-off-by: Danny Cao <dcao@us.ibm.com>
@caod123 caod123 force-pushed the fab-17466 branch 2 times, most recently from 2a15d4a to 6086aa9 Compare February 20, 2020 14:52
@caod123
Copy link

caod123 commented Feb 20, 2020

Fixed a couple other things I noticed in the tests (alongside a parallel fix for FAB-17515). This PR is now holding up 3-4 other PRs dependent on this being merged. Seeing as how this has already been thoroughly reviewed within the squad, we've talked to Matt already and he's agreed we can go ahead and merge this and he'll come back to this with any followup comments after he returns. @denyeart could you go ahead and merge this barring any comments you may have?

@denyeart
Copy link
Contributor

Sure, looks like comments addressed, merging.

@denyeart denyeart merged commit 9eaab49 into hyperledger:master Feb 20, 2020
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.

None yet

5 participants