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
Bug 1830270: cmd: add explain subcommand #3515
Bug 1830270: cmd: add explain subcommand #3515
Conversation
abhinavdahiya
commented
Apr 28, 2020
otherwise the |
65b203b
to
cda31e0
Compare
cda31e0
to
1f77e5c
Compare
1c6a05b
to
cbbf437
Compare
|
||
## Additional guidelines on godoc comments | ||
|
||
All definitions in installer codebase already follow and enforce [effective-go] guidelines, but for better explain output for the types these additional guidelines should also be followed |
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.
missing an anchor for effective-go
?
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.
@wking fixed
docs/dev/explain.md
Outdated
|
||
3. The comments on the fields must use the `json` tag to reference the field. | ||
|
||
4. Optional fields must have the `+optional` marker defined. Optional fields must also define the default value. If the values is static, `+kubebuilder:validation=Default` marker should be used, otherwise the comment must clearly define how the default value will be calculated. |
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.
nit: "values is" -> "value is" or "values are"
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.
@wking fixed
/retitle Bug 1830270: cmd: add explain subcommand |
@abhinavdahiya: This pull request references Bugzilla bug 1830270, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sdodson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Noted a couple of typos.
pkg/explain/cmd.go
Outdated
Long: `This command describes the fields associated with each supported InstallConfig API. Fields are identified via a simple | ||
JSONPath identifier: | ||
|
||
isntallconfig.<fieldName>[.<fieldName>] |
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.
isntallconfig
-> installconfig
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.
@patrickdillon fixed
pkg/explain/cmd.go
Outdated
|
||
raw, err := ioutil.ReadAll(file) | ||
if err != nil { | ||
return errors.Wrap(err, "faield to read InstallConfig CRD") |
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.
faield
-> failed
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.
@patrickdillon fixed
cbbf437
to
2b02482
Compare
/skip |
LGTM |
go generate ./pkg/types/installconfig.go
The explain package provides an `explain` cobra command that can use used to add it as subcommand. The package includes 2 major functions, - fields_lookup Given a list of paths, this finds the schema for the path from the install config schema. - printer The printer printers the schema provided to the function The printer always prints the InstallConfig apiVersion, kind to the provided io.Writer It prints the description of the field using the schema provided, and for object type, it also prints all the properties of the object and their descriptions.
2b02482
to
4d03a03
Compare
@patrickdillon rebased the PR on master |
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@abhinavdahiya: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@abhinavdahiya: All pull requests linked via external trackers have merged: openshift/installer#3515. Bugzilla bug 1830270 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |