-
Notifications
You must be signed in to change notification settings - Fork 94
feat: pass invocation_mode
property to uploadDeployFunction
endpoint
#454
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
Conversation
✅ Deploy Preview for open-api ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Sorry, I realised I was still missing the piece for passing the field to the deploy API call. Done in 72453ba. |
go/porcelain/deploy.go
Outdated
@@ -781,10 +781,11 @@ func bundleFromManifest(ctx context.Context, manifestFile *os.File, observer Dep | |||
}) | |||
} | |||
|
|||
if function.DisplayName != "" || function.Generator != "" { | |||
if function.DisplayName != "" || function.Generator != "" || function.InvocationMode != "" { |
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.
hmm, is this condition correct? Can you have a function config without display name or generator, just the invocation mode?
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.
You can. None of the fields are mandatory. I think the check is just preventing us from writing an empty function config object, which in itself feels weird but I didn't want to change it as part of this PR.
Okay, slight change of plans! In db268d4 I've moved the new |
|
||
// Path OR Buffer should be populated | ||
Path string | ||
Buffer io.ReadSeeker | ||
} | ||
|
||
type FunctionMetadata struct { |
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've added a struct because I can imagine us adding more function-specific properties in the future, and since FileBundle
is shared with static files, it'd be a bit odd to pollute it with a bunch of function-specific properties.
In the future, it would make sense to also move Runtime
to this struct, as it's also specific to functions.
invocation_mode
to functions configinvocation_mode
property to uploadDeployFunction
endpoint
go/porcelain/deploy.go
Outdated
@@ -781,6 +793,7 @@ func bundleFromManifest(ctx context.Context, manifestFile *os.File, observer Dep | |||
}) | |||
} | |||
|
|||
// TODO: Move these to the `meta` object above. |
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.
assuming this is a todo in the future
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.
Yes, but actually it's not clear we definitely want to do this, so I've removed the comment in 3fab5e4.
return err | ||
} | ||
} | ||
|
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.
nit: this empty line is not needed, same as line 225
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 code is automatically generated by Swagger, so it's not something we can control. 😢
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.
ohh that makes sense 🤦
Pass
invocation_mode
to theuploadDeployFunction
endpoint as a URL query parameter.