Prepare for chaining autogenerated conversion methods #7431

wojtek-t · 2015-04-28T12:35:05Z

Part of #6800

This PR is prerequisite for chaining autogenerated conversion methods. The next steps are:

test checking whether auto-generated code wasn't manually edited
call auto-generated methods one from another [this is easy modulo effective handling of defaulting functions]

wojtek-t · 2015-04-28T13:22:19Z

@lavalamp @smarterclayton
To be clear on calling auto-generated methods one from another. The issue with it is that defaulting functions, are performed on reflections:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/conversion/converter.go#L462
So far, I have no idea how to avoid reflections for that, so if you have any suggestions please let me know.

[update]
I think it might be doable if we will be storing the defaulting functions as "interface{}" instead of "reflect.Value". Then, while generating code we will know how to cast the given interface. So I think that should work. But please let me know if you have any better suggestions.

smarterclayton · 2015-04-28T14:12:25Z

In this change, please add instructions on generation to docs/api_changes.md so that folks needing to add things to the API (@liggitt) have some place to go.

wojtek-t · 2015-04-28T14:33:30Z

In this change, please add instructions on generation to docs/api_changes.md so that folks needing to add things to the API (@liggitt) have some place to go.

I've started writing this doc and uploaded few lines of it. However, I might not be able to finish it today. So it would be great if you could take a look at the code, before I'm done with the document.

smarterclayton · 2015-04-28T14:42:45Z

Separate the conversion changes into their own commit so it's easier to review. We probably want to formalize that in the policy around handling these (always put generations in a separate commit).

wojtek-t · 2015-04-28T14:52:09Z

Separate the conversion changes into their own commit so it's easier to review. We probably want to formalize that in the policy around handling these (always put generations in a separate commit).

Done

smarterclayton · 2015-04-28T15:00:04Z

Instead of

    err := newer.Scheme.AddGeneratedConversionFuncs(
        func(in *AWSElasticBlockStoreVolumeSource, out *newer.AWSElasticBlockStoreVolumeSource, s conversion.Scope) error {
            return convertV1beta3AWSElasticBlockStoreVolumeSourceToApiAWSElasticBlockStoreVolumeSource(in, out, s)
        },

do

    err := newer.Scheme.AddGeneratedConversionFuncs(
        convertV1beta3AWSElasticBlockStoreVolumeSourceToApiAWSElasticBlockStoreVolumeSource,

wojtek-t · 2015-04-28T15:00:47Z

I'm going to improve how the cmd/kube-conversion/conversion.go is used in this PR too (tomorrow), but this won't change the code that is generated. So it would be great if you could look into that. Thanks!

smarterclayton · 2015-04-28T15:01:26Z

Instead of convertV1beta3AWSElasticBlockStoreVolumeSourceToApiAWSElasticBlockStoreVolumeSource can you do convert_v1beta3_AWSElasticBlockStoreVolumeSource_To_internal_AWSElasticBlockStoreVolumeSource

smarterclayton · 2015-04-28T15:01:45Z

What's the golang limit on function names?

thockin · 2015-04-28T15:19:59Z

docs/api-changes.md

@@ -0,0 +1,22 @@
+API Changes


we already have a docs/devel/api_changes.md - merge?

Thanks @thockin - sounds good to me. I will merge those two files (most probably tomorrow).

lavalamp · 2015-04-28T18:07:55Z

pkg/conversion/generator.go

@@ -27,6 +27,7 @@ import (
 type Generator interface {
 	GenerateConversionsForType(version string, reflection reflect.Type) error
 	WriteConversionFunctions(w io.Writer) error
+	WriteConversionFunctionsForRegister(w io.Writer) error


What does "ForRegister" mean?

This wasn't the best name. Fixed.

wojtek-t · 2015-04-29T08:57:19Z

Instead of convertV1beta3AWSElasticBlockStoreVolumeSourceToApiAWSElasticBlockStoreVolumeSource can you do convert_v1beta3_AWSElasticBlockStoreVolumeSource_To_internal_AWSElasticBlockStoreVolumeSource

I think that names without underscores ale closer to the styleguide (I didn't see any names with underscores anywhere). Why do you want to change this?

All the other comments are applied - PTAL

smarterclayton · 2015-04-29T12:33:00Z

On Apr 29, 2015, at 4:57 AM, Wojciech Tyczynski notifications@github.com wrote:

Instead of convertV1beta3AWSElasticBlockStoreVolumeSourceToApiAWSElasticBlockStoreVolumeSource can you do convert_v1beta3_AWSElasticBlockStoreVolumeSource_To_internal_AWSElasticBlockStoreVolumeSource

I think that names without underscores ale closer to the styleguide (I didn't see any names with underscores anywhere). Why do you want to change this?

The style guide really doesn't apply to generated functions. The fully transformed version is almost unreadable, so the underscores bring it back.

I'd be willing to make an exception in our style guide to improve the readability and scanability of the list. Also, it preserves the original casing of the names which is probably better.

All the other comments are applied - PTAL

—
Reply to this email directly or view it on GitHub.

wojtek-t · 2015-04-29T12:39:31Z

The style guide really doesn't apply to generated functions. The fully transformed version is almost unreadable, so the underscores bring it back.
I'd be willing to make an exception in our style guide to improve the readability and scanability of the list. Also, it preserves the original casing of the names which is probably better.

I don't fully agree with that, but I see your point. Changed.

PTAL

wojtek-t · 2015-04-29T14:17:29Z

BTW - I have almost ready the next PR, that is calling those conversion functions one from another without using reflections. That PR gives us another ~75% improvements (which is more than 10x from the original results), but since that new PR depends on this one, I will wait with submitting that one before this one is merged.

smarterclayton · 2015-04-29T14:19:39Z

One more nag - please alpha sort the conversion function references in

newer.Scheme.AddGeneratedConversionFuncs(
convert_v1beta3_AWSElasticBlockStoreVolumeSource_To_api_AWSElasticBlockStoreVolumeSource,

So you can quickly binary search visually to find them. Will make it easier to identify missed conversion bugs and diffs. Best order is a forced order.

wojtek-t · 2015-04-29T14:36:02Z

One more nag - please alpha sort the conversion function references in
newer.Scheme.AddGeneratedConversionFuncs(
convert_v1beta3_AWSElasticBlockStoreVolumeSource_To_api_AWSElasticBlockStoreVolumeSource,
So you can quickly binary search visually to find them. Will make it easier to identify missed conversion bugs and diffs. Best order is a forced order.

Done - PTAL

smarterclayton · 2015-04-29T14:41:12Z

This is LGTM barring any final review from @lavalamp

wojtek-t · 2015-04-29T14:42:46Z

Thanks @smarterclayton !

lavalamp · 2015-04-29T16:24:59Z

My feedback is that it's not obvious which sections of the file are autogenerated and which aren't (add comments before the functions?) and that maybe you could generate it for v1, too, to keep that in sync?

lavalamp · 2015-04-29T16:26:24Z

But I don't think that needs to block the PR.

wojtek-t · 2015-04-29T16:39:10Z

My feedback is that it's not obvious which sections of the file are autogenerated and which aren't (add comments before the functions?)

I definitely agree with it - tomorrow I will be working on the test whether the generated code wasn't manually edited and I will add it as part of it.

and that maybe you could generate it for v1, too, to keep that in sync?

Can I do it in a separate PR?

But I don't think that needs to block the PR.

If you're fine with merging as is, that would be great. I will address your two comments in a separate PR tomorrow.

Prepare for chaining autogenerated conversion methods

googlebot added the cla: yes label Apr 28, 2015

wojtek-t force-pushed the conversion_chains branch from 16d45a6 to 0078456 Compare April 28, 2015 14:30

wojtek-t force-pushed the conversion_chains branch 2 times, most recently from 9f66589 to 6135d99 Compare April 28, 2015 14:41

wojtek-t force-pushed the conversion_chains branch from 6135d99 to 8cf7a69 Compare April 28, 2015 14:51

wojtek-t force-pushed the conversion_chains branch from 8cf7a69 to 9a6f3df Compare April 28, 2015 14:58

thockin reviewed Apr 28, 2015
View reviewed changes

a-robinson assigned smarterclayton Apr 28, 2015

lavalamp reviewed Apr 28, 2015
View reviewed changes

wojtek-t force-pushed the conversion_chains branch 3 times, most recently from b06a158 to b890642 Compare April 29, 2015 08:52

wojtek-t force-pushed the conversion_chains branch from b890642 to f19ada7 Compare April 29, 2015 12:38

wojtek-t added 2 commits April 29, 2015 16:32

Improvements to conversions generator.

8eca762

Generate conversions

fc6fb09

wojtek-t force-pushed the conversion_chains branch from f19ada7 to fc6fb09 Compare April 29, 2015 14:34

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2015

fgrzadkowski added a commit that referenced this pull request Apr 30, 2015

Merge pull request #7431 from wojtek-t/conversion_chains

2312841

Prepare for chaining autogenerated conversion methods

fgrzadkowski merged commit 2312841 into kubernetes:master Apr 30, 2015

wojtek-t deleted the conversion_chains branch April 30, 2015 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for chaining autogenerated conversion methods #7431

Prepare for chaining autogenerated conversion methods #7431

wojtek-t commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

thockin Apr 28, 2015

wojtek-t Apr 28, 2015

lavalamp Apr 28, 2015

wojtek-t Apr 29, 2015

wojtek-t commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

lavalamp commented Apr 29, 2015

lavalamp commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

Prepare for chaining autogenerated conversion methods #7431

Prepare for chaining autogenerated conversion methods #7431

Conversation

wojtek-t commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

wojtek-t commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

smarterclayton commented Apr 28, 2015

thockin Apr 28, 2015

Choose a reason for hiding this comment

wojtek-t Apr 28, 2015

Choose a reason for hiding this comment

lavalamp Apr 28, 2015

Choose a reason for hiding this comment

wojtek-t Apr 29, 2015

Choose a reason for hiding this comment

wojtek-t commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

wojtek-t commented Apr 29, 2015

lavalamp commented Apr 29, 2015

lavalamp commented Apr 29, 2015

wojtek-t commented Apr 29, 2015