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

InsertPreviousObjects assumes APIs have already been generated #471

Open
negz opened this issue Feb 13, 2025 · 1 comment
Open

InsertPreviousObjects assumes APIs have already been generated #471

negz opened this issue Feb 13, 2025 · 1 comment
Labels
bug Something isn't working

Comments

@negz
Copy link
Member

negz commented Feb 13, 2025

What happened?

I'm prototyping a change to upjet that'd have it generate namespaced API types and controllers. See #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 #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 #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 negz added the bug Something isn't working label Feb 13, 2025
@negz
Copy link
Member Author

negz commented Feb 13, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant