SDN docs and stuff #10143
SDN docs and stuff #10143
Conversation
| type ClusterNetwork struct { | ||
| unversioned.TypeMeta `json:",inline"` | ||
| // Standard object's metadata. | ||
| kapi.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` | ||
|
|
||
| // Network is a CIDR string to specify the global overlay network's L3 space | ||
| // network is a CIDR string specifying the global overlay network's L3 space |
There was a problem hiding this comment.
I think we want to keep these as uppercase to be valid Godoc as that's more important than it being consistent with the generated swaggerdoc for the json field name
There was a problem hiding this comment.
@stevekuznetsov @smarterclayton explicitly requested lowercase field names in docs in #9227 (comment). (Though it seems that only pkg/build currently follows that convention.)
There was a problem hiding this comment.
@smarterclayton that's surprising ... why do we want this?
There was a problem hiding this comment.
This Godoc is turned into swagger, which is read by humans in the context of JSON responses. Someone reading swagger will never see the Go types, they will only ever see the JSON responses. Returning the Go type name is thus not useful to them, while the JSON field name is.
There was a problem hiding this comment.
Right, but why don't we want Godoc to read correctly for developers using the lib? I think the correct approach here would be to edit the comments programmatically when we generate Swagger descriptions from them. It doesn't feel right to break Godoc conventions to appease some code generator downstream.
|
Other than the comments from @stevekuznetsov , LGTM. |
|
LGTM |
openshift-bot
added
the
needs-rebase
danwinship
force-pushed
the
sdn-docs-and-stuff
branch
from
c002f1e
to
b7d116b
Compare
openshift-bot
removed
the
needs-rebase
|
[test] |
openshift-bot
added
the
needs-rebase
danwinship
force-pushed
the
sdn-docs-and-stuff
branch
from
b7d116b
to
0913137
Compare
|
Repushed without the uppercase/lowercase changes. Should I file a bug about making the doc generation process fix up the field names automatically? |
|
Evaluated for origin test up to 0913137 |
|
@danwinship sure, the tool lives upstream so file it against k8s please |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7908/) |
openshift-bot
removed
the
needs-rebase
|
@stevekuznetsov done. ok to merge? |
|
LGTM, has @smarterclayton signed off? |
|
Looks good to me. |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8385/) (Image: devenv-rhel7_4916) |
|
Evaluated for origin merge up to 0913137 |
follow-ups from review comments on #9227:
oc describedidn't support the SDN types@openshift/networking PTAL