-
Notifications
You must be signed in to change notification settings - Fork 4
[config] Add Orderer endpoint #5
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
Conversation
5dba24c to
e03c91f
Compare
|
LGTM. |
pasquale95
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
e03c91f to
50e07de
Compare
50e07de to
30d6b68
Compare
91c102c to
455540a
Compare
Signed-off-by: Liran Funaro <[email protected]>
455540a to
f1539ae
Compare
cendhu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have a suggestion but I am merging this PR.
| // Schema 1: YAML. | ||
| // Schema 2: JSON. | ||
| // Schema 3: [id=ID,][msp-id=MspID,][broadcast,][deliver,][host=Host,][port=Port,][Host:Port]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about supporting multiple schema formats, as too many choices are not always good. It would be better to support only the YAML schema within the YAML config file. Further, it would reduce the number of lines of code that we need to manage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support at least two schemas. Scehma 1 (YAML, to the request of @pasquale95), and schema 3 to support regular endpoint for legacy.
Type of change
Description
yaml.v3