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

Update doc for k8s.io/code-generator/cmd/conversion-gen #71821

Merged

Conversation

@MikeSpreitzer
Copy link
Member

commented Dec 6, 2018

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR updates the documentation for k8s.io/code-generator/cmd/conversion-gen. The old doc did not explain the k8s:conversion-gen-external-types comment tag and did not explain how developers can selectively override generated conversion functions.

Which issue(s) this PR fixes:

Special notes for your reviewer:
This was discussed in Slack starting at https://kubernetes.slack.com/archives/C0EG7JC6T/p1542037685235100

Does this PR introduce a user-facing change?:

NONE
@MikeSpreitzer

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

/assign @caesarxuchao

@caesarxuchao
Copy link
Member

left a comment

@MikeSpreitzer Kudos, it's much better than the current comment. I asked for a few changes.

/assign @sttts

Show resolved Hide resolved staging/src/k8s.io/code-generator/cmd/conversion-gen/main.go Outdated
// that efficiently convert between same-name types in each of the two
// packages. The generated functions include ones named
// autoConvert_<pkg1>_<type>_To_<pkg2>_<type>
// for each such ordered pair of types. If the destination package

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Dec 21, 2018

Member

Could you clarify what you mean by "ordered pair"?

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Dec 29, 2018

Author Member

Reworded.

//
// Given a list of input directories, it will scan for "peer" packages and

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Dec 21, 2018

Member

Could you also update the comments in Makefile.generated_files that mentions the "peer" packages?

IIUC, peer packages is the package with the internal types.

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Dec 21, 2018

Member

Hmm, the conversion-gen also has --base-peer-dirs and --extra-peer-dirs flags. Can we keep the terms consistent?

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Dec 29, 2018

Author Member

Updated to discuss the "peer" role and the effects of those flags.

We are stuck between a rock and a hard place here. The directories that are technically "input" directories (according to the general code gen framework) play only one fixed role in conversion code generation: they are where the outputs go. I have tried to thread this rhetorical needle by first defining terms that make sense for conversion code generation, and then relating those terms to the ones used in the general code generation framework.

Also edited that makefile.

// // +k8s:conversion-gen=<import-path-of-peer-package>
// Given a list of input directories, `conversion-gen` will scan for
// comment tags that define conversion tasks. A package may request
// conversion generation by including a comment in the file-comments

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Dec 21, 2018

Member

"a comment in the file-comments of one file", do you mean the "package comment"? If so, using "package comment" since it's terse.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Dec 29, 2018

Author Member

Updated to reflect the behavior and comments in the current code.

// request Conversion generation by including a comment in the file-comments of
// one file, of the form:
// // +k8s:conversion-gen=<import-path-of-peer-package>
// Given a list of input directories, `conversion-gen` will scan for

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Dec 21, 2018

Member

Is the "list of input directories" the value specified via the "--input-dirs" flag? If so, can we explicitly say so?
Something like "To start code generation, invoke the conversion-gen binary with flag --input-dirs=<input directories>. conversion-gen will scan for ..."

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Dec 29, 2018

Author Member

Updated.

// between internal and external types. A general conversion task
// involves three packages: the one containing the internal types, the
// one containing the external types, and the destination (i.e., the
// one containing the conversion functions).

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Dec 21, 2018

Member

I think "i.e, where the generated conversion functions are output to" is more clear. WDYT?

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Dec 29, 2018

Author Member

I wrote it the way I did because that package is not only where the generated conversion functions go but is also where the developer-authored conversion functions appear.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Dec 29, 2018

Author Member

Updated, to include the suggested wording and also mention the developer-authored functions.

@MikeSpreitzer MikeSpreitzer force-pushed the MikeSpreitzer:fix-conversion-gen-doc branch 2 times, most recently from d14b8b1 to 51e417f Dec 29, 2018

@sttts

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

This is so much better than anything we before about conversion. Let's get this in.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 17, 2019

@spiffxp

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

FWIW this lgtm but I can't /approve

@cblecker

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, MikeSpreitzer, sttts

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

Update doc for k8s.io/code-generator/cmd/conversion-gen
Added explanation of the `k8s:conversion-gen-external-types` comment
tag.

Added explanation of how the developer can selectively override the
generated conversion functions.

Also updated description in Makefile.generated_files.

@MikeSpreitzer MikeSpreitzer force-pushed the MikeSpreitzer:fix-conversion-gen-doc branch from 51e417f to 4982cbb Jan 28, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 28, 2019

@MikeSpreitzer

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2019

I added a note about how Scheme.AddConversionFuncs must also be called in order for developer-maintained conversions to be used in the case of top-level object types.

@sttts

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 28, 2019

@spiffxp

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

FWIW pull-kubernetes-e2e-gce failing due to kubernetes/test-infra#11001 and retests won't help that (though ideally they will eventually work once that's resolved)

@fejta-bot

This comment has been minimized.

Copy link

commented Jan 29, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@cblecker

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

/retest

@nikhita

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Jan 29, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

@MikeSpreitzer: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 4982cbb link /test pull-kubernetes-e2e-kops-aws

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.

@k8s-ci-robot k8s-ci-robot merged commit 8e69630 into kubernetes:master Jan 29, 2019

17 of 18 checks passed

pull-kubernetes-e2e-kops-aws Job failed.
Details
cla/linuxfoundation MikeSpreitzer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@MikeSpreitzer MikeSpreitzer deleted the MikeSpreitzer:fix-conversion-gen-doc branch Feb 2, 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.