upjet icon indicating copy to clipboard operation
upjet copied to clipboard

`InsertPreviousObjects` assumes APIs have already been generated

Open negz opened this issue 10 months ago • 1 comments

What happened?

I'm prototyping a change to upjet that'd have it generate namespaced API types and controllers. See https://github.com/crossplane/upjet/pull/470.

I'm testing my change with provider-upjet-aws. Part of that involves generating a completely new apis-namespaced directory. When I run make generate upjet panics when calling versionGen.InsertPreviousObjects:

panic: cannot insert type definitions from the previous versions into the package scope for group "lakeformation.aws.upbound.io": cannot load the previous versions of "aws_lakeformation_permissions" from path github.com/upbound/provider-aws/apis-namespaced/lakeformation/v1beta1: err: chdir /home/negz/control/crossplane-contrib/provider-upjet-aws/apis-namespaced/lakeformation/v1beta1: no such file or directory: stderr:

I've spent all day staring at the versionGen.InsertPreviousObjects (introduced in https://github.com/crossplane/upjet/pull/402) but I'm having a lot of trouble understanding what it does. It seems to me that the implementation doesn't match its Godoc or the description in https://github.com/crossplane/upjet/pull/402.

I think there's an order dependency issue:

  1. When generating API Go types for (e.g.) guardduty v1beta1
  2. If any guardduty resources have v1beta1 as a previous version
  3. Then try to load types from the v1beta1/zz_*_types.go files for all guardduty resources into package scope

However at the time InsertPreviousObjects is called there's no guarantee that v1beta1/zz_*_types.go files exist for all guardduty resources. In fact the first time code generation runs, they definitely won't exist.

How can we reproduce it?

In provider-upjet-aws delete all the generated files (and their directories, but not e.g. generate.go) under apis/ and run make generate.

negz avatar Feb 13 '25 02:02 negz

It seems like if I remove the panic from the error case everything just works fine? At least, the exact same API types are generated whether or not the error case is hit. I've tested this by:

  1. Changing the panic in the InsertPreviousObjects error case to fmt.Println
  2. Running make generate to generate into an empty apis-namespaced/ using my PR above - this logs many errors
  3. Creating a git commit of the generated code
  4. Running make generate again

The second make generate doesn't create a diff against the git commit, which indicates that the same files are generated whether the error case is hit or not.

negz avatar Feb 13 '25 02:02 negz