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

Attempt to port a serverless component to this library #17

Closed
wants to merge 2 commits into from

Conversation

lukehoban
Copy link
Contributor

This is the beginnings of an attempt to use this library to use this library to port a slightly non-trivial component. A few questions/issues that came up - curious if there are thoughts on these, or features that need to be added to support these:

  1. Ran into Support references to external types #16 trying to use external types in outputs (but was able to remove this temporarily to unblock further issues).
  2. The use of a locally defined FunctionMapInput is not supported - it seems like only Input/Output wrappers over built-in pulumi SDK types are allowed. How are nested types supported? I see in the provider implementations that plain types are used, so these aren't needed. But in the components, they seem to (rightly?) expect to work with inputs/outputs that are Inputs/Outputs. But then how do I use nested types within this?
  3. Related - note that I have to copy the same giant types.go file used in https://github.com/pulumi/pulumi-package-serverless/ (which I am porting from). Not everything there is strictly necessary - but a lot of it seems to be. Is there a better way to author this with this model? Note that in the original variant of this, I actually wrote schema first, then generated SDK, then stole the SDK types back into the implementation to use for the implementation of the Construct. Convoluted for sure - but with this amount of nested types - authoring in schema was actually quite a lot simpler than in Go types. Can this be easier with the higher level framework here?
  4. Not yet directly related to this - but would be lovely to have testing support built into the examples - a main_test.go that runs ProgramTest with a YAML program (so that SDK isn't needed for inner loop testing)??

Currently this is blocked on (2) above - resulting in:

Error: input Functions for property main.Service has type main.FunctionMapInput, which is not a valid input type

@github-actions
Copy link

github-actions bot commented Jul 7, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project

@lukehoban
Copy link
Contributor Author

cc @iwahbe @harribo11 for thoughts on questions/notes above.

@iwahbe
Copy link
Member

iwahbe commented Jul 7, 2022

  1. We can and will implement this. The problem is deriving the underlying type given a InputT or OutputT, but this is doable. We have a design and just need to implement it.
  2. If I'm understanding correctly, this is unsupported for the same reason as (1) and will be fixed in the same PR.
  3. That is definitely a sharp edge with this approach. I link against the generated SDK in my approach, which I need to bootstrap to generate. I need to do some spikes to see if I can come up with an ergonomic approach to this. (Ergonomic way to reference provider defined types in component resource #18)
  4. Yes it would. (Add Pulumi YAML based tests to the examples #19)

@hliuson
Copy link
Contributor

hliuson commented Jul 7, 2022

Concur with Ian on points 1 and 2. Will our token system work across all providers? For example the docker provider has type tokens formatted docker:index/container:Container. If not then we will probably need to add some way to handle those edge cases.

@lukehoban lukehoban force-pushed the lukehoban/serverless branch from 63f4ae1 to e88f8b7 Compare July 9, 2022 00:03
@github-actions
Copy link

github-actions bot commented Jul 9, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project

@github-actions
Copy link

github-actions bot commented Jul 9, 2022

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project

Comment on lines +62 to +68
component := &Service{}
// What about opts?
err := ctx.RegisterComponentResource("serverless:index:Service", name, component)
if err != nil {
return err
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
component := &Service{}
// What about opts?
err := ctx.RegisterComponentResource("serverless:index:Service", name, component)
if err != nil {
return err
}

s is the component that you are registering here. You don't need to create a new component or register it. We don't expose Options, since they should all be inherited by parenting s. Let me know if you can think of a reason we need to expose Options.

Comment on lines +210 to +218
component.Url = out.ApplyT(func(v interface{}) *string {
return v.(*string)
}).(pulumi.StringPtrOutput)

if err := ctx.RegisterResourceOutputs(component, pulumi.Map{
"url": component.Url,
}); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
component.Url = out.ApplyT(func(v interface{}) *string {
return v.(*string)
}).(pulumi.StringPtrOutput)
if err := ctx.RegisterResourceOutputs(component, pulumi.Map{
"url": component.Url,
}); err != nil {
return err
}
s.Url = out.ApplyT(func(v interface{}) *string {
return v.(*string)
}).(pulumi.StringPtrOutput)

@iwahbe
Copy link
Member

iwahbe commented Jan 7, 2025

Closing, since this is out of date.

@iwahbe iwahbe closed this Jan 7, 2025
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.

3 participants