-
Notifications
You must be signed in to change notification settings - Fork 12
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
Validate that resource import ids are not outputs #608
Conversation
708e73a
to
07df31d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
importValue, ok := e.evaluateExpr(v.Options.Import) | ||
if ok { | ||
if !hasOutputs(importValue) { | ||
opts = append(opts, pulumi.Import(pulumi.ID(v.Options.Import.Value))) | ||
} else { | ||
e.error(v.Options.Import, "import must be not be an output") | ||
overallOk = false | ||
} | ||
} else { | ||
e.error(v.Options.Import, "couldn't evaluate the 'import' resource option") | ||
overallOk = false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be done statically. This should not happen during runtime.
Given the TODO in the PR, this code should be unreachable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true but I don't think the type checker today distinguishes between prompt/plain values and outputs. Does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It would need to be a different pass then the type checker.
Closing this PR as the approach isn't correct. Tracked by #614 . |
A program that uses the resource import option with an
Output
as the import id:currently fails like this:
error: failed to discover plugin requirements: Pulumi.yaml:15,15-22: import must be a string.
This error message is somewhat misleading, although correct in a narrow sense (outputs are not strings).This PR adds a check to detect that case and fail with a better message.
TODO: The added test does not work as intended so far: it doesn't even get to the new code in
registerResource
but fails earlier inLoadYAMLBytes
, with the previous "import must be a string" error.