Refactoring and cleanup in preparation for changing namespaces #2139
Conversation
theunrepentantgeek
requested review from
davefellows,
Porges,
matthchr and
babbageclunk
as code owners
v2/tools/generator/internal/codegen/pipeline/create_resource_extension_types.go
Show resolved
Hide resolved
...xtendedResourceFunction_moreThanOneVersion_GeneratesExpectedCode/GetExtendedResources.golden
Outdated
Show resolved
Hide resolved
...den_GetExtendedResourceFunction_oneVersion_GeneratesExpectedCode/GetExtendedResources.golden
Outdated
Show resolved
Hide resolved
| // | ||
| // func(this *TypeVisitor, it *ResourceType, ctx interface{}) (Type, error) | ||
| // func(it *ResourceType) (Type, error) | ||
| // func(it *ResourceType) Type |
There was a problem hiding this comment.
This reads sorta weird to me. Up above we list (basically) these same signatures, then down here we repeat them? It might make more sense to put this example up above with the TypeName example? Not that another example hurts but it's not super clear to me how this example is different than what line 17 is already saying, which is "Any of these formats can work but replace TypeName with any supported type?
There was a problem hiding this comment.
I got confused by the comment and used the wrong kind of signature; will try another way to clarify.
| // functions) from the golden test output. | ||
| // These golden tests are looking at the structure of the generated structs; including all the code for functions in | ||
| // the output is just noise that makes the golden files hard to review. | ||
| func stripFunctionsPipelineStage() *pipeline.Stage { |
There was a problem hiding this comment.
I don't think that this is true. Look at https://github.com/Azure/azure-service-operator/tree/main/v2/tools/generator/internal/codegen/testdata/ArmResource for example. These tests expect to include the various validation functions and ARM conversion functions. This stage seems to leave most of those (I'm not entirely sure why - maybe has to do with where the stage is injected into the pipeline?) It does seem to remove some stuff about // +kubebuilder:rbac:groups=test.azure.com,resources=fakeresources,verbs=get;list;watch;create;update;patch;delete, which I think we want in the golden files as (AFAIK) there is no other place that is tested?
If the objective here is just to remove certain interfaces/functions because they're already tested elsewhere, wouldn't it be easier to just remove the select set of stages we know weve tested? That allows us per-stage control here and we can just prune the stages we've already tested elsewhere and leave the rest?
There was a problem hiding this comment.
We can't just pull those stages.
For one, they're often prerequisites of other stages that we do want to test.
For another, a lot of our stages are ONLY tested by these golden tests (they don't have isolated unit tests of their own). The more our test pipeline resembles the actual one we use for code generation the better, because we rule out unexpected interactions between the stages.
As discussed offline, I'll remove the filtering for now, and have logged #2150 so we remember to circle back and make things better.
| // We do this by constructing a local package reference because this avoids replicating the logic here and risking | ||
| // inconsistency if things are changed in the future. | ||
| local := astmodel.MakeLocalPackageReference("prefix", "group", astmodel.GeneratorVersionPrefix, version.name) | ||
| _, lv, _ := local.GroupVersion() |
There was a problem hiding this comment.
Should we panic rather than discard here? And below?
There was a problem hiding this comment.
Added, though I don't see how it would ever fire.
There was a problem hiding this comment.
Yeah agreed, neither do I. It pays to be paranoid sometimes though - who knows in the future when "That cannot possibly happen" can actually start happening because of some other refactor.
Thanks for adding!
There was a problem hiding this comment.
Reactivating as I don't think you actually did this at this location? (You did it below though I see)
theunrepentantgeek
force-pushed
the
feature/back-compat-refactoring
branch
from
b562dd9 to
6b91a0f
Compare
| func (stage *Stage) checkPostrequisites(state *State) error { | ||
| var errs []error | ||
| for _, postreq := range stage.postrequisites { | ||
| early := state.stagesSeen.Contains(postreq) |
There was a problem hiding this comment.
Should this also check stagesExpected?
There was a problem hiding this comment.
Discussed offline; we check stagesExpected at the end of the pipeline, to check there are no further stages pending.
| // func(this *TypeVisitor, it TypeName, ctx interface{}) (Type, error) | ||
| // func(it TypeName) (Type, error) | ||
| // func(it TypeName) Type | ||
| // func(this *TypeVisitor, it <sometype>, ctx interface{}) (Type, error) |
There was a problem hiding this comment.
Ha, I had a comment suggesting this!
theunrepentantgeek
force-pushed
the
feature/back-compat-refactoring
branch
from
0b49d4a to
4ba23c1
Compare
Codecov Report
@@ Coverage Diff @@
## main #2139 +/- ##
==========================================
+ Coverage 57.16% 57.18% +0.02%
==========================================
Files 591 591
Lines 109522 109591 +69
==========================================
+ Hits 62609 62675 +66
- Misses 39369 39372 +3
Partials 7544 7544
Continue to review full report at Codecov.
|
| @@ -133,7 +133,7 @@ typeTransformers: | |||
| property: Copy | |||
| ifType: | |||
| group: deploymenttemplate | |||
| version: v1alpha1api20190401 | |||
| version: "2019-04-01" | |||
There was a problem hiding this comment.
minor: Can this just be 2019-04-01? Does it need the quotes?
There was a problem hiding this comment.
Needs the quotes, else it seems to get interpreted as a date.
| // | ||
| // Some examples: | ||
| // | ||
| // VisitTypeName = func(it TypeName) Type // Works |
There was a problem hiding this comment.
minor: Maybe include 1 more // Works example too at the bottom? Possibly that's one of the larger format ones like
func(it *ObjectType) (Type, error) or func(this *TypeVisitor, it *ObjectType, ctx interface{}) (Type, error). I think that helps accomplished what you were going for before of showing examples for TypeName and other fields. One that is a ptr (since TypeName isn't) probably?
| // | ||
| // For example, InjectJsonSerializationTests creates round trip serialization tests for any object types that have | ||
| // properties. It's not correct to give InjectJsonSerializationTests a prerequisite on every earlier stage that creates | ||
| // new object types becauses it isn't concerned with where those object came from. However, but those earlier stages DO |
There was a problem hiding this comment.
Minor/wording: However, but on this line is phrased strangely? maybe just:
| // new object types becauses it isn't concerned with where those object came from. However, but those earlier stages DO | |
| // new object types becauses it isn't concerned with where those object came from. However, those earlier stages DO |
| // We do this by constructing a local package reference because this avoids replicating the logic here and risking | ||
| // inconsistency if things are changed in the future. | ||
| local := astmodel.MakeLocalPackageReference("prefix", "group", astmodel.GeneratorVersionPrefix, version.name) | ||
| _, lv, _ := local.GroupVersion() |
There was a problem hiding this comment.
Reactivating as I don't think you actually did this at this location? (You did it below though I see)
What this PR does / why we need it:
Refactoring and cleanup in preparation for changing our namespace prefixes from from
v1alpha1apitov1beta.To avoid having to mess around with string manipulation when creating compatibility types (coming in a future PR), changed
LocalPackageReferenceto explicitly receive the prefix as a separate parameter.vso they won't change when the production prefix is changedStoragePackageReferenceto more consistently defer to the wrappedLocalPackageReferencecontainsPreviewVersionLabel()to skip our fixed prefixtest.MakeLocalPackageReference()from testsConversion graph generation was prevously driven by the API types - but we don't have those available for generation of compatibility types.
generate the conversion graph from the storage types (since each
StoragePackageReferencecontains within it the linked LocalPackagReference`).Remove dependency of
CreateStorageTypeson the conversion graphMoving
CreateStorageTypesahead ofCreateConversionGraphin the pipeline unearthed a number of problems with our pipeline and it's testing. Verification of pre- and post-requisities wasn't being done for tests, resulting in some tests having incorrect object structurespipeline.Stage, including tests.pipeline.State.RunTestPipeline()more consistently when testing pipeline stagesCode gardening and diagnostics
Special notes for your reviewer:
How does this PR make you feel:
If applicable: