Implement the Convertible interface on non-hub resources #1628

theunrepentantgeek · 2021-07-07T05:35:44Z

What this PR does / why we need it:

Implements the required ConvertTo() and ConvertFrom() methods needed by the conversions.Convertible interface used by the controller

Note that the stage is disabled for two reasons. First is that we don't want to activate this functionality until post codegen-alpha-0. Second is that we need to turn on a bunch of features together, otherwise the internal validation of the controller will reject our webhooks.

Prerequisites

Merge PR Convert Direction from enum into a discriminated union #1618
Merge PR Move Property Assignment Function into the functions package #1622
Merge PR Create conversion graph #1627
Merge PR Introduce Pipeline State #1644
Rebase on master

How does this PR make you feel:

If applicable:

this PR contains tests

matthchr

Some minor comments but looks good overall

hack/generator/pkg/astmodel/interface_injector.go

matthchr · 2021-07-20T16:56:03Z

hack/generator/pkg/codegen/pipeline/implement_convertible_interface.go

+			injector := astmodel.NewInterfaceInjector()
+
+			modifiedTypes := make(astmodel.Types)
+			resources := storage.FindResourceTypes(state.Types())


I feel like this method (and the others in types_tools that are generic) should be moved to astmodel along with your interface/function injectors?

Reading this made me think it was finding the storage resource types or something, I had to go look at the impl to understand it was just any old resource type.

Your FindStatusTypes could probably replace my findAllResourceStatusTypes in remover.go as well, although we could leave that for another PR because it'll require changing some types around I think.

Agreed. I'm sure moved these into astmodel in a later PR, not sure which one off the top of my head.

Would you consider pulling it forward? I've got at least one upcoming PR where I'd like to use the Status one.

hack/generator/pkg/codegen/pipeline/implement_convertible_interface.go

matthchr · 2021-07-20T16:59:41Z

hack/generator/pkg/codegen/pipeline/implement_convertible_interface_test.go

+	"github.com/Azure/azure-service-operator/hack/generator/pkg/test"
+)
+
+func TestInjectConvertibleInterface(t *testing.T) {


Should this test be replaced with a standard json input file + golden file once the stages are actually enabled in the pipeline?

If so, consider leaving a TODO here to that effect?

No, the purpose of this test is to test this pipeline stage in isolation (well, as much isolation as possible). I don't want to conflate the results of this test with other things happening to independent stages.

Added a comment.

matthchr · 2021-07-20T17:06:47Z

hack/generator/pkg/astmodel/interface_implementation.go

@@ -57,6 +57,11 @@ func (iface *InterfaceImplementation) References() TypeNameSet {
 	return results
 }

+// FunctionCount returns the number of included functions
+func (iface *InterfaceImplementation) FunctionCount() int {


Would it make more sense to just expose this as Functions() like exists on ResourceType? (Can we do that since this is also embedded there?)

Functions returns a copy (by necessity), whereas this can return the count directly. Not that we're too performance sensitive, but that's why I added. it. 😁

hack/generator/pkg/functions/resource_conversion_function_test.go

matthchr · 2021-07-20T17:14:03Z

hack/generator/pkg/functions/resource_conversion_function.go

+//
+// Direct conversion from the hub type:
+//
+// func (r <receiver>) ConvertFrom(hub conversion.Hub) error {


minor: use the From|To notation you used below to clarify that this can also happen in either direction?

Or maybe because you aren't using it super consistently below either (and doing so might make the examples awkward for example in the error messages, etc) instead you could just preface this with something like: "Examples show the From direction. The only difference between the two is that all instances of From are replaced with To in the To direction." or something?

I've been wavering on whether including <From|To> actually adds clarity or just makes things harder to read. Adding them in for now.

Yeah FWIW I agree. In theory I like <From|To> but in practice it makes things a bit harder to read. I think my main thing here is just be consistent, if you don't use them that's fine you can just have a preface or something that explains roughly how the other direction might look. The comments are meant to help us understand the code rather than be an exact, faithful replica of the template that is being applied so I think either is ok.

At the moment I'm leaning (just) towards keeping them in - it's still easier to parse the comment with <To|From> than the actual code, and it cues readers into the complexity that's present.

hack/generator/pkg/functions/resource_conversion_function.go

…rces

codecov-commenter · 2021-07-21T05:01:55Z

Codecov Report

Merging #1628 (fa8ac7e) into master (b7ccaba) will increase coverage by 0.39%.
The diff coverage is 87.38%.

@@            Coverage Diff             @@
##           master    #1628      +/-   ##
==========================================
+ Coverage   67.22%   67.61%   +0.39%     
==========================================
  Files         208      211       +3     
  Lines       14979    15175     +196     
==========================================
+ Hits        10069    10260     +191     
  Misses       4151     4151              
- Partials      759      764       +5

Impacted Files	Coverage Δ
hack/generator/pkg/astmodel/types.go	`62.80% <ø> (ø)`
hack/generator/pkg/codegen/code_generator.go	`89.03% <ø> (ø)`
hack/generator/pkg/astmodel/property_injector.go	`80.00% <66.66%> (ø)`
hack/generator/pkg/astmodel/interface_injector.go	`72.22% <72.22%> (ø)`
...odegen/pipeline/implement_convertible_interface.go	`81.81% <81.81%> (ø)`
hack/generator/pkg/astmodel/function_injector.go	`90.47% <84.61%> (ø)`
...ator/pkg/functions/resource_conversion_function.go	`91.24% <91.24%> (ø)`
...generator/pkg/astmodel/interface_implementation.go	`65.71% <100.00%> (+2.07%)`	⬆️
...erator/pkg/codegen/pipeline/inject_hub_function.go	`80.95% <100.00%> (ø)`
...g/codegen/pipeline/inject_original_gvk_function.go	`76.19% <100.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7ccaba...fa8ac7e. Read the comment docs.

theunrepentantgeek requested review from babbageclunk, davefellows, matthchr and Porges as code owners July 7, 2021 05:35

theunrepentantgeek self-assigned this Jul 7, 2021

theunrepentantgeek added this to the codegen-alpha-1 milestone Jul 7, 2021

theunrepentantgeek force-pushed the feature/convertible-resources branch from cf37d2b to 813f382 Compare July 7, 2021 05:41

theunrepentantgeek changed the base branch from master to feature/create-conversion-graph July 11, 2021 23:44

theunrepentantgeek force-pushed the feature/create-conversion-graph branch from b2b1ec5 to 60d27fd Compare July 11, 2021 23:48

theunrepentantgeek force-pushed the feature/convertible-resources branch from 92f2e98 to 7a5ba94 Compare July 11, 2021 23:49

theunrepentantgeek mentioned this pull request Jul 12, 2021

Eliminate most use of hard coded strings for pipeline stage names #1640

Merged

4 tasks

theunrepentantgeek force-pushed the feature/create-conversion-graph branch from df32912 to e9e04cf Compare July 14, 2021 02:50

theunrepentantgeek force-pushed the feature/convertible-resources branch from 7a5ba94 to 8890b7e Compare July 14, 2021 03:30

theunrepentantgeek mentioned this pull request Jul 14, 2021

Implement conversions for Specs and Statuses #1649

Merged

5 tasks

theunrepentantgeek force-pushed the feature/convertible-resources branch from 8890b7e to 994c3b5 Compare July 17, 2021 23:37

theunrepentantgeek force-pushed the feature/create-conversion-graph branch from 3520a61 to 9a876ce Compare July 17, 2021 23:51

theunrepentantgeek force-pushed the feature/convertible-resources branch from 994c3b5 to 55dd583 Compare July 17, 2021 23:53

matthchr approved these changes Jul 20, 2021

View reviewed changes

Base automatically changed from feature/create-conversion-graph to master July 21, 2021 02:49

theunrepentantgeek force-pushed the feature/convertible-resources branch from 64e9d88 to 131ec1b Compare July 21, 2021 03:01

theunrepentantgeek added 9 commits July 21, 2021 15:05

Create ResourceConversionFunction and test

a978fae

Allow injection of multiple functions at once

0883b1f

Code gardening

472f91b

Move FunctionInjector into astmodel

7073068

Move PropertyInjector into astmodel

5082aec

Create InterfaceInjector in astmodel

179728a

Create pipeline stage to implement the Convertible interface on resou…

82cf36b

…rces

Integrate into pipeline

66d41b2

Code Gardening

cc7ade0

theunrepentantgeek added 3 commits July 21, 2021 15:05

Mark methods as internal

3d0587e

Update ImplementConvertibleInterface stage for new pipeline state

fe69aba

Updates base on PR feedback

3fb22f1

theunrepentantgeek force-pushed the feature/convertible-resources branch from 131ec1b to 3fb22f1 Compare July 21, 2021 03:06

theunrepentantgeek added 2 commits July 21, 2021 16:35

Fix lint issues

aceb5b2

Update golden files

fa8ac7e

theunrepentantgeek merged commit 6667d97 into master Jul 21, 2021

theunrepentantgeek deleted the feature/convertible-resources branch July 21, 2021 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the Convertible interface on non-hub resources #1628

Implement the Convertible interface on non-hub resources #1628

theunrepentantgeek commented Jul 7, 2021 •

edited

Loading

matthchr left a comment

matthchr Jul 20, 2021

theunrepentantgeek Jul 21, 2021

matthchr Jul 21, 2021

matthchr Jul 20, 2021

theunrepentantgeek Jul 21, 2021

theunrepentantgeek Jul 21, 2021

matthchr Jul 20, 2021

theunrepentantgeek Jul 21, 2021

matthchr Jul 20, 2021

matthchr Jul 20, 2021

theunrepentantgeek Jul 21, 2021

matthchr Jul 21, 2021

theunrepentantgeek Jul 21, 2021 •

edited

Loading

codecov-commenter commented Jul 21, 2021

Implement the Convertible interface on non-hub resources #1628

Implement the Convertible interface on non-hub resources #1628

Conversation

theunrepentantgeek commented Jul 7, 2021 • edited Loading

matthchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theunrepentantgeek Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 21, 2021

Codecov Report

theunrepentantgeek commented Jul 7, 2021 •

edited

Loading

theunrepentantgeek Jul 21, 2021 •

edited

Loading