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

Fix emitting PANIC in generated programs when we don't have access to the schema of resource properties #1344

Closed
wants to merge 1 commit into from

Conversation

Zaid-Ajaj
Copy link
Contributor

Description

Fixes #1341

The problem here is when we don't have access to a pulumi schema during program-gen, then currentResourcePropertyType from the generator type is nil. Then at some point we ask for typeName(g.currentResourcePropertyType) and call .String() on the schema type which is nil causing a PANIC to be generated. This PR simply makes sure that currentResourcePropertyType is initialized to a non-nil value to avoid the panic.

That is only half of the problem of #1341 though, because we should have access to the schema when generating examples in the docs. That is however not something we can fix from the point of view of pulumi-java

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@Zaid-Ajaj Zaid-Ajaj requested review from t0yv0 and a team April 30, 2024 20:19
Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. Can we add a conformance tests in pu/pu for this? I know Java isn't running conformance tests yet but this sounds like a valid use case that we should make sure we're checking for all the languages.

@t0yv0
Copy link
Member

t0yv0 commented May 1, 2024

This looks reasonable. I added a lot more tickets today around these PANICS they do not all have the same root cause it appears.

For the root cause that "I don't know the schema here" the provider build use case would actually be lovely to error out with exactly that error message, so we know that we missed a reference to a provider in our build environment.

In addition I seem to hit cases like the one recently fixed in pulumi-random where the AWS provider is referenced in the build environment but at the wrong version (v5 vs v6), so some of the schemas for resources do not load. That'd be nice to know too..

Having actionable warnings is a nice to have of course, for now I just would love to find a way to close out all the actual AWS PANIC issues.

@lunaris
Copy link
Contributor

lunaris commented May 7, 2024

After digging into the issues that Anton raised, I think that the correct fix for several of them is to error when the schema lookup fails, rather than trying to continue and avoid the panic. Specifically, for:

the examples are incorrect/contain mis-named attributes, which cause the schema lookup failures. This appears to be the approach that other languages take (and indeed, looking at the other language conversions for the examples Anton gave, many fail due to the examples being incorrect). As a result I'm going to raise another PR and recommend we close this one.

@justinvp justinvp closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PANIC generated into public-facing documentation
5 participants