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

Redesign ResourceOptions family #307

Closed
lbialy opened this issue Nov 22, 2023 · 8 comments
Closed

Redesign ResourceOptions family #307

lbialy opened this issue Nov 22, 2023 · 8 comments
Labels
area/api User visible API area/core The SDK's core code awaiting-feedback Cannot progress until more feedback is gathered impact/breaking Fixing this issue will require a breaking change impact/broken Something that is difficult or impossible for some people to use impact/reliability Something that feels unreliable or flaky kind/improvement An improvement with existing workaround needs-design Needs from design work: architecture, API, DX/UX size/M Estimated effort to complete (up to 5 days).
Milestone

Comments

@lbialy
Copy link
Collaborator

lbialy commented Nov 22, 2023

This issue captures all of the issues that are present with current design of ResourceOptions. CRO is currently quite... unwieldy. This structure is used extremely often and yet it requires user to type CustomResourceOptions or ComponentResourceOptions to set a single field in it. This is bad DX. To make things worse arguments on most RO are:

  • broken (like Custom Timeouts for instance, the type used - String - does not represent them at all and does not work in runtime
  • have a non-flexible argument type (for instance dependsOn takes an Output[List[Resource]] which forces user to .map(List(_)) all single resource cases and also .zip to do the same with multiple resources)
  • use old argument syntax with | NotProvided union whereas codegenerated libs have all moved to Input[A]

The inspiration to improve this comes, surprisingly, from the pulumi-go sdk. In pulumi-go user does not create a struct to pass it to the resource constructor. On the contrary, the DX is quite interesting (it's definitely not without issues!) - resource constructor, after the resource name argument and args argument expects any amount of ResourceOption via a variadic argument. Here are some examples:

db, err := NewDatabase(ctx, "db", &DatabaseArgs{ /*...*/ },
    pulumi.AdditionalSecretOutputs([]string{"password"}))
res1, _ := NewMyResource(ctx, "res1", &MyResourceArgs{/*...*/})
res2, _ := NewMyResource(ctx, "res2", &MyResourceArgs{/*...*/}, pulumi.DependsOn([]pulumi.Resource{res1}))

Each of these functions creates an instance of ResourceOption (notice the lack of plural s!) that is later folded onto the struct that is instantiatated in pulumi-go code and not exposed to the user. This is quite interesting and definitely improves otherwise tricky go DX. To compare this with python, which Scala's SDK is probably closest syntactically to:

db = Database('db',
    opts=ResourceOptions(additional_secret_outputs=['password']))
res1 = MyResource("res1")
res2 = MyResource("res2", opts=ResourceOptions(depends_on=[res1]))

I'm not exactly sure if variadic argument solution is exactly what we should go towards here mostly because these options would then need to pollute our top-level import scope. For instance for resources that have optional Args like Namespace:

val db = Namespace("db",  opts = additionalSecretOutputs("a", "b"), dependsOn(k8sProvider, someOtherResource))

This syntax apparently work right now so it's only a matter of decision whether we want resource options as free functions imported via global import besom.*:

@ def dependsOn(resource: Any, otherResources: Any*): String = s"dependsOn=($resource${otherResources.mkString(", ", ", ", "")})"
defined function dependsOn

@ def additionalSecretOutputs(member: String, otherMembers: String*): String = s"additionalSecretOutputs=($member${otherMembers.mkString(", ", ", ", "")})"
defined function additionalSecretOutputs

@ def NamespaceArgs(name: String): String = s"name=$name"
defined function NamespaceArgs

@ def Namespace(name: String, args: Option[Any] = None, opts: Any*): Unit = println(s"name: $name${args.fold("args: default")(a => s", args: $args")}, opts: ${opts.mkString(", ")}")
defined function Namespace

// here we go:

@ val ns1 = Namespace("my-app")
name: my-appargs: default, opts:

@ val ns2 = Namespace("my-app-2", Some(NamespaceArgs(name = "my-ns")))
name: my-app-2, args: Some(name=my-ns), opts:

@ val ns3 = Namespace("my-app-3", opts = dependsOn(resourceA, resourceB), additionalSecretOutputs("a", "b", "c"))
name: my-app-3args: default, opts: dependsOn=(resource A, resource B), additionalSecretOutputs=(a, b, c)

@ val ns4 = Namespace("my-app-4", Some(NamespaceArgs(name = "my-ns-2")), opts = dependsOn(resourceA, resourceB), additionalSecretOutputs("a", "b", "c"))
name: my-app-4, args: Some(name=my-ns-2), opts: dependsOn=(resource A, resource B), additionalSecretOutputs=(a, b, c)

The most important bit here is that we can skip optional args by using the named argument syntax and all further arguments go into variadic position of other resource options.

@lbialy
Copy link
Collaborator Author

lbialy commented Nov 22, 2023

@pawelprazak @prolativ can I get some input on this?

@lbialy
Copy link
Collaborator Author

lbialy commented Nov 22, 2023

I can already see that with our usual scalafmt formatting this is gonna become a little uglier:

val ns4 = Namespace(
  "my-app-4",
  Some(NamespaceArgs(name = "my-ns-2")), 
  opts = dependsOn(resourceA, resourceB), 
  additionalSecretOutputs("a", "b", "c"))
)

Notice opts = on line 4 but lack of the same on line 5. We can't have multiple named args in Scala, compiler disallows it.

@lbialy lbialy added impact/broken Something that is difficult or impossible for some people to use kind/improvement An improvement with existing workaround area/core The SDK's core code size/M Estimated effort to complete (up to 5 days). area/api User visible API needs-design Needs from design work: architecture, API, DX/UX impact/breaking Fixing this issue will require a breaking change awaiting-feedback Cannot progress until more feedback is gathered impact/reliability Something that feels unreliable or flaky labels Nov 22, 2023
@pawelprazak
Copy link
Contributor

IMHO, the non-supprising python-esqu API opts=ResourceOptions(depends_on=...) is a good starting point.
I personally, find the function based API surprising. Would it play well with autocomplete form an IDE?

If we get feedback that this simple API is an issue for the users, we could try some tricks.

I'd certainly take into account the tooling support for any non-boring API, because sometimes it's easier to use an API that is verbose and not clever, but it works well with the tooling available.

If in doubt, keep is boring and simple.

@lbialy
Copy link
Collaborator Author

lbialy commented Nov 22, 2023

Do you also find it surprising in pulumi-go?

About tooling - it is different in that there's no starting point, IDE won't know what you can put there (because for some reason they don't take into account available function return types against what is expected in given position) so it will propose all the functions in the scope. In the end I guess it boils down to the fact that you have to have knowledge about what you want to use beforehand so discoverability sucks. On the other hand typing CustomResourceOptions almost every time is bad, even with autocomplete and copilot. It's pure noise, doesn't do anything, it's just a long-ass type and we can't have ResourceOptions because we're typesafe and there are multiple variants of ResourceOptions (that's actually top type of an ADT). I have another proposal then, one that doesn't incur the discoverability penalty and is based on @prolativ's Scala 3 endeavours - neomagnet pattern!

// in core sdk
// in reality they are all a bit different and need different arguments
@ enum ResourceOptions:
      case ProviderResourceOptions(dependsOn: List[Any] = Nil, additionalSecretOutputs: List[String] = Nil)
      case CustomResourceOptions(dependsOn: List[Any] = Nil, additionalSecretOutputs: List[String] = Nil)
      case ComponentResourceOptions(dependsOn: List[Any] = Nil, additionalSecretOutputs: List[String] = Nil)
  import ResourceOptions.*
defined class ResourceOptions
import ResourceOptions.*

@ {
  sealed trait ResourceOptsVariant:
    type Constructor
    val constructor: Constructor
  class Provider extends ResourceOptsVariant:
    type Constructor = ProviderResourceOptions.type
    val constructor = ProviderResourceOptions
  class Component extends ResourceOptsVariant:
    type Constructor = ComponentResourceOptions.type
    val constructor = ComponentResourceOptions
  class Custom extends ResourceOptsVariant:
    type Constructor = CustomResourceOptions.type
    val constructor = CustomResourceOptions
  }
defined trait ResourceOptsVariant
defined class Provider
defined class Component
defined class Custom

// this is the most important bit
@ def opts(using variant: ResourceOptsVariant): variant.Constructor = variant.constructor
defined function opts

// codegenerated
@ def Namespace(name: String, args: Option[String] = None, opts: Custom ?=> CustomResourceOptions): Unit = println(opts(using Custom()))
defined function Namespace

// user's code
// old syntax still works as before
@ val n = Namespace("test", Some("arguments"), CustomResourceOptions(dependsOn=List("dependency")))
CustomResourceOptions(List(dependency),List())

// but there's also a shorthand opts() available!
@ val n2 = Namespace("test", Some("arguments"), opts(dependsOn=List("dependency")))
CustomResourceOptions(List(dependency),List())

This allows user to disregard the whole CustomResourceOptions garbage and just type opts() and it will resolve to a correct apply method and have identical IDE support as using full CustomResourceOptions (so the same possible arguments for function application in IDE).

@pawelprazak
Copy link
Contributor

Do you also find it surprising in pulumi-go?

Yes, most of the Go SDK was surprising to me.

This allows user to disregard the whole CustomResourceOptions garbage and just type opts() and it will resolve to a correct apply method and have identical IDE support as using full CustomResourceOptions (so the same possible arguments for function application in IDE).

Yes, this sounds promissing, but would have to test it to be sure :)

@lbialy lbialy added this to the 0.2.0 milestone Jan 11, 2024
@pawelprazak
Copy link
Contributor

the #336 contains a proposition for implementing this pattern

@lbialy
Copy link
Collaborator Author

lbialy commented Jan 25, 2024

I'm not sure whether we want to proceed with this go-like syntax in the end or just refactor ResourceOptions so that they may use neomagnet function opts? What do you think after #336?

@pawelprazak
Copy link
Contributor

Yes, neomagnet function opts sound good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api User visible API area/core The SDK's core code awaiting-feedback Cannot progress until more feedback is gathered impact/breaking Fixing this issue will require a breaking change impact/broken Something that is difficult or impossible for some people to use impact/reliability Something that feels unreliable or flaky kind/improvement An improvement with existing workaround needs-design Needs from design work: architecture, API, DX/UX size/M Estimated effort to complete (up to 5 days).
Projects
None yet
Development

No branches or pull requests

2 participants