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

[cli/import] - Handle panic during import codegen #7265

Merged
merged 3 commits into from Jun 11, 2021
Merged

Conversation

komalali
Copy link
Member

@komalali komalali commented Jun 10, 2021

Description

This PR is to handle a panic that is caused by a failure during go codegen when using the pulumi import command. Unfortunately, import is impossible to write tests for since it is dependent on a resource existing in a provider. The original PR to add the import command included no tests.

I have also been unable to repro this particular failure manually since I do not have the correct access to use the PushRestrictions feature for BranchProtection in Github as I do not have an organization using the team or enterprise tiers.

I tested this change by manually adding a panic to the generateImportedDefinitions function and observing the resulting output.

Related to: #7112

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

@komalali komalali changed the title [cli/import] - Handle panic during codegen [cli/import] - Handle panic during import codegen Jun 10, 2021
@komalali komalali marked this pull request as ready for review June 10, 2021 19:01
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

handle a panic that is caused by a failure during go codegen

Is there a reason we can't have codegen return a real error instead of panicng? It wasn't entirely clear here what call in the code here is panicing, and why that is expected such that we need to add a panic handler at this layer?

Assuming there is a good need to still be panicing in the codegen, this change certainly looks like it helps improve the UX for pulumi import.

Unfortunately, import is impossible to write tests for since it is dependent on a resource existing in a provider. The original PR to add the import command included no tests.

Technically I believe we could import a resource like random.RandomString which doesn't have any backing cloud provider state (or have a "dummy" resource provider that we test against). But I agree we can tackle test coverage here independently of this PR.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
@komalali
Copy link
Member Author

komalali commented Jun 10, 2021

It wasn't entirely clear here what call in the code here is panicing, and why that is expected such that we need to add a panic handler at this layer?

So, this is the line that panics:

panic(errors.Errorf("invalid Go source code:\n\n%s", index.String()))

You're right though, it should just return an error. The reason that I didn't fix it at that layer is because docgen has existing code to work around this, and I wanted to avoid working in multiple layers and potentially alter the behavior of existing tooling to get to resolution here.

The reason we're getting to this panic, in this case, is not at the codegen layer, it's something that happens higher up the chain (possibly with the readResource call and the state generated) but I have been unable to investigate that since I don't have the correct access to this resource to observe the panic myself.

Technically I believe we could import a resource like random.RandomString which doesn't have any backing cloud provider state (or have a "dummy" resource provider that we test against). But I agree we can tackle test coverage here independently of this PR.

Yes, but there is no guarantee that any of those imports would lead to the error that we see here, which I can't get to a root cause for because of lack of access to the resource.

In any case, I was trying to resolve the panic without diving into multiple other layers when I can't even repro the error.

Co-authored-by: Luke Hoban <luke@pulumi.com>
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.

None yet

3 participants