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-17176] Make decode hooks consistent in viperutil #424

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

stephyee
Copy link
Contributor

Signed-off-by: Tiffany Harris tiffany.harris@ibm.com

Type of change

Improvement to code

Related issues

https://jira.hyperledger.org/browse/FAB-17176

Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>
continue
}
result = append(result, string(pem.EncodeToMemory(block)))
if block.Type != "CERTIFICATE" || len(block.Headers) != 0 {
Copy link
Contributor

@yacovm yacovm Dec 16, 2019

Choose a reason for hiding this comment

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

I know this is like the original form, but - why do we really enforce a CERTIFICATE header here?
This is a generic viper util, so what if we have here a public key or a private key ? Then the header is different.

Also, i don't think that PEM parsing cares about header type at all - it only cares about -- BEGIN and -- END prefixes and ---- suffixes.

Served as "food for thought" only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing all of 'viperutil' is on our TODO list. I think we'd be better off without the magic (but not quite correct or consistent magic) around things like PEM blocks.

Copy link
Contributor

@jyellick jyellick left a comment

Choose a reason for hiding this comment

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

LGTM

continue
}
result = append(result, string(pem.EncodeToMemory(block)))
if block.Type != "CERTIFICATE" || len(block.Headers) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing all of 'viperutil' is on our TODO list. I think we'd be better off without the magic (but not quite correct or consistent magic) around things like PEM blocks.

@jyellick jyellick merged commit 2fbd83d into hyperledger:master Dec 17, 2019
@stephyee stephyee deleted the fab-17176 branch December 17, 2019 16:18
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

3 participants