-
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
Allow version to be set dynamically #510
base: main
Are you sure you want to change the base?
Conversation
pkg/pulumiyaml/run.go
Outdated
ctx.addErrDiag(node.Key.Syntax().Syntax().Range(), | ||
"Version conflicts with the default provider version", | ||
fmt.Sprintf("Try removing this option on resource \"%s\"", k)) | ||
stringValue := func(expr ast.Expr) string { |
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.
I feel like this function maybe needs access to an evaluator such that it can evaluate the version if it is set dynamically but I am not sure how we go about it or whether it is 100% necessary 🤔 cc @iwahbe @AaronFriel thoughts?
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 function as used on L254 will definitely need an evaluator.
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.
Updated the setDefaultProviders
function to take an evaluator and actually get the value from the visited resources ✅
resources: | ||
provider-a: | ||
type: pulumi:providers:test | ||
version: ${version} | ||
provider-b: |
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.
Shouldn't something later in this test assert that provider-a is actually registered with version "1.0.0"?
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.
Yeah you are right, will add it to the test
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.
Added an assertion that the registered provider has the version evaluated correctly ✅
if v == nil || v.Value == "" { | ||
return nil, nil | ||
} | ||
func ParseVersion(expr ast.Expr) (*semver.Version, error) { |
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.
Is this even used anymore? If we're allowing version to be any expression it probably isn't safe to have a function laying around that looks like it correctly gives a *semver.Version from an Expr.
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 is used in a few places where there isn't an evaluator available to evaluate the expression, so it tries to get the value of the version when it is statically known (i.e. *ast.StringExpr
).
it probably isn't safe to have a function laying around that looks like it correctly gives a *semver.Version from an Expr
The function isn't total, it will try to return a *semver.Version
when it can from a string expression, otherwise it will return nil
which the rest of the caller code handles already (when resolving resource/invoke versions)
Maybe adding a bool
to the returned tuple could make the signature a bit more clear. Returning false
when the Expr
is anything but *ast.StringExpr
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.
Are we allowing version
to be any expression, or just a non-literal? Specifically, are we allowing version
to be a outputty value?
I would strongly recommend that version
must be knowable at preview time.
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.
Syntactically it can be any expression but the evaluation should return string
at runtime. We check that when we run evaluateExpr
:
if versionExpr, ok := e.evaluateExpr(t.CallOpts.Version); ok {
if version, ok := versionExpr.(string); ok {
opts = append(opts, pulumi.Version(version))
} else {
e.error(t.CallOpts.Version, "version must be a string value")
}
}
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.
Let's make sure that there is an issue to update the documentation on this change then.
pkg/pulumiyaml/analyser.go
Outdated
var version *semver.Version | ||
if v.Options.Version != nil { | ||
switch versionValue := v.Options.Version.(type) { | ||
case *ast.StringExpr: | ||
parsedVersion, err := ParseVersion(versionValue) | ||
if err != nil { | ||
ctx.error(v.Type, fmt.Sprintf("unable to parse resource %v provider version: %v", k, err)) | ||
return true | ||
} | ||
version = parsedVersion |
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.
I'm nervous about this for two reasons:
- We don't actually check that
v.Options.Version
is typed as a string, so it might evaluate to some other type later. Have you tried runningpulumi up
against this branch and passing in a value of another type here? - Correct version information is necessary to type check correctly. If we fail to resolve the correct version we could reject programs that should work or vice versa.
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.
Updated such that the type checker computes the type of the Version
and makes sure it is assignable to schema.StringType
. Then at runtime when we evaluate Version
, if it has outputs, we return an error.
pkg/pulumiyaml/run.go
Outdated
ctx.addErrDiag(node.Key.Syntax().Syntax().Range(), | ||
"Version conflicts with the default provider version", | ||
fmt.Sprintf("Try removing this option on resource \"%s\"", k)) | ||
stringValue := func(expr ast.Expr) string { |
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 function as used on L254 will definitely need an evaluator.
if v == nil || v.Value == "" { | ||
return nil, nil | ||
} | ||
func ParseVersion(expr ast.Expr) (*semver.Version, error) { |
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.
Are we allowing version
to be any expression, or just a non-literal? Specifically, are we allowing version
to be a outputty value?
I would strongly recommend that version
must be knowable at preview time.
pkg/pulumiyaml/run.go
Outdated
} else { | ||
e.error(t.CallOpts.Version, "version must be a string value") | ||
} |
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 should be an internal error. It version resolves to something not a string, then the type checker should have caught 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.
Yeah added a type-check for Version
such that it must be assignable to string, so removed this one ✅
overallOk = false | ||
} | ||
} else { | ||
e.error(v.Options.Version, "version must be not be an output") |
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.
I don't remember if the type checker can catch this kind of thing, but if it can that would be great.
88eb37a
to
bdbb737
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.
I think we need a test of what happens if you try and set the expression to something that resolves to unknown
at preview time. But otherwise this looks ok.
Fixes #508
This PR changes the definition of
Version
from a static*ast.StringExpr
toast.Expr
so that it can a value which can be evaluated at runtime when running the program rather than just static strings. Anywhere else where want to know the version and we don't have an evaluator yet, we check if the version is a static string and get its value, otherwise we skip it.