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

Create `genyaml` library to generate commented YAML from Go structs. #13637

Merged
merged 9 commits into from Aug 14, 2019

Conversation

@clarketm
Copy link
Member

commented Jul 26, 2019

The genyaml tool is used to generate commented YAML snippets from Go structs. It decorates the YAML with doc comments parsed from .go source files.

The first (proposed) implementation of this tool will be to dynamically generate plugins.yaml snippets for each of the Prow plugins in an effort to unblock PR : #13507 and resolve issue: #9079. Due to the size and modularity of this tool, I felt that it was appropriate to create it separate from its implementation.

Reference the associated README for a more detailed description and usage.

@clarketm

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

@k8s-ci-robot k8s-ci-robot requested review from cjwagner and stevekuznetsov Jul 26, 2019

@clarketm

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

/assign @stevekuznetsov

@cjwagner
Copy link
Member

left a comment

I didn't review everything in detail just yet, but this looks pretty cool!
A couple requests:

  • Please add the vendored dependencies in a separate commit.
  • I think we should place this library outside of Prow and avoid making it specific to plugin configuration (consider the pkg dir). This seems generally useful for other parts of Prow and other tools. If we make it easy to use anywhere it will encourage us to make our tools provide commented config examples and expose commented versions of the current config.
    • Maybe even consider upstreaming this to the yaml library?
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml_test.go Outdated
Show resolved Hide resolved prow/plugins/config.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
Show resolved Hide resolved prow/genyaml/genyaml.go Outdated
@alvaroaleman
Copy link
Member

left a comment

This is super, super awesome and extremely useful even for projects completely outside of prow

Show resolved Hide resolved prow/genyaml/genyaml_test.go Outdated

@clarketm clarketm force-pushed the clarketm:genyaml branch 5 times, most recently from adfe307 to f64bd50 Jul 29, 2019

@clarketm

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@cjwagner @alvaroaleman @stevekuznetsov - All comments have been addressed. Thanks for the feedback! Please review.

/assign @cjwagner

@clarketm clarketm force-pushed the clarketm:genyaml branch 2 times, most recently from 4fc7436 to e1b56ae Aug 1, 2019

clarketm added some commits Aug 1, 2019

@clarketm clarketm force-pushed the clarketm:genyaml branch 3 times, most recently from b9b5900 to c083a70 Aug 1, 2019

@clarketm clarketm force-pushed the clarketm:genyaml branch 2 times, most recently from 415a292 to d9dd0e7 Aug 1, 2019

clarketm added some commits Aug 1, 2019

@clarketm clarketm force-pushed the clarketm:genyaml branch from d9dd0e7 to 8d166a5 Aug 1, 2019

@clarketm

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/test pull-test-infra-bazel

@cblecker
Copy link
Member

left a comment

started reviewing this but didn't finish. will continue when I have time

Show resolved Hide resolved pkg/genyaml/genyaml.go
@alvaroaleman
Copy link
Member

left a comment

Hey, thanks for the work and sorry for the long delay!

When I tried it out there was unfortunately a panic, see below.

Would it be possible as a follow-up to build this in a way that we can just feed it .go files and it will emit docs? That would be a lot nicer from an UX perspective. Afaik the various go linters have code to read and analyse go code

Show resolved Hide resolved pkg/genyaml/genyaml.go Outdated
@Katharine
Copy link
Member

left a comment

Very impressive! A few comments.

Show resolved Hide resolved pkg/genyaml/genyaml.go Outdated
Show resolved Hide resolved pkg/genyaml/genyaml.go Outdated
Show resolved Hide resolved pkg/genyaml/genyaml.go Outdated
Show resolved Hide resolved pkg/genyaml/genyaml.go
Show resolved Hide resolved pkg/genyaml/genyaml.go Outdated
Show resolved Hide resolved pkg/genyaml/testdata/multiline_comments/multiline_comments.yaml Outdated
Show resolved Hide resolved pkg/genyaml/testdata/omit_if_empty/example_config.go Outdated
Show resolved Hide resolved pkg/genyaml/testdata/pointer_types/pointer_types.yaml
@clarketm

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

LGTM label has been added.

Git tree hash: 2c9b0fc5e3e425212eb467a6d64c227f8c7cc4c4

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clarketm, Katharine

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 9981dc3 into kubernetes:master Aug 14, 2019

5 of 6 checks passed

tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation clarketm authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped.
pull-test-infra-verify-file-perms Job succeeded.
Details
pull-test-infra-yamllint Job succeeded.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.