API Design Review

[lukehoban]

A few topics in particular:

• Package name
• Resource names
• Should create/delete have environment/interpreter nested under them or at top level
• Should there be a property to force replace? (Like random’s keepers)
• Should create/delete force replace?
• More…

[iwahbe]

To answer the original questions

Package name

I would prefer something like system (or os) for the package name. I think system.Command is more expressive then command.Command. This is similar to how other langauges model commands, indiciating that the behaviour is platform specific. It also provides a clear place to add other system/os level hooks (like filesystem modeling). I don't feel strongly on this point though.

Resource names

I might be missing a trick for one of the languages we support, but could we consider

• "command:index:Command" -> "command:local:Command"
• "command:index:RemoteConnection" -> "command:remote:Connection"
• "command:index:RemoteCommand" -> "command:remote:Command"

Since we have two types of commands, I think it would make sense to model these as two namespaces.

Should create/delete have environment/interpreter nested under them or at top level

I'm concerned that this will make the API noisy. Is is possible we could have some kind of "merge and" method? It would work like

const provision = new local.Command("cleanup", {
    dir: "$HOME/dir",
    update: "rm detritus",
    delete: "rm -rf ."
}).and(new local.Command("setup", {
    create: "mkdir $HOME/dir"
}));

Here "provision" would tie together the effects of "cleanup" and "setup" into one resource. The method would probably return a resource reference that lists both "provision" and "cleanup" as dependencies. You could rely on "provision" and that would in tern invoke both "cleanup" and "setup".

Should there be a property to force replace? (Like random’s keepers)

I don't entirely understand keepers. Could you explain why this would be helpful.

Should create/delete force replace?

I'm not sure I understand this either. Do you mean that create != "" || delete != "" implies replace for all updates? I don't agree with that. I imagine that Command would be used to manage the state of an external (to pulumi) resource. We might want to perform updates separate from replaces. If we needed to we could add a input: "alwaysReplace". I would not add this unless it becomes necessary (is requested by users).

I don't think that replace should ever be used for command (unless explicitly requested). Imagine a simple resource to model the life cycle of a folder.

const folder = new Command("folder", {
    create: "mkdir /folder",
    update: "echo [pulumi up] $(time) >> /folder/log.txt"
    delete: "rm -r /folder"
});

• pulumi up creates the folder.
• pulumi up --replace $(urn-of folder) creates a new folder (running create before delete) which fails because the folder already exists. If we ignore this failure, we would then delete the folder, cleaning up the old resource. At this point, /folder does not exist.
• pulumi up would run the update hook (since pulumi believes the resource already exists), which would fail (writing to a folder that does not exist).

Other design questions I had

Exec vs Shell

We discussed security concerns before. One way to make the system more secure is to allow/force exec style arguments. We could provide an interface where each argument is a separate string. This ameliorates the need for input sanitization.

If an interpreter was desired, ["/bin/bash", "-c", args.join(" ")] would revert back to the previous behavior.

We could expose both, but for security reasons, I would prefer to expose only the exec style.

Exit Codes and Failure

I am assuming that the command will fail if a non-zero exit code is returned. Is it work handling non-zero exit codes. I am good with failing on non-zero exit, but I wanted to confirm that it was a conscious decision.

Providers

Another way to model inheritable environmental state (enviroment, dir, interpreter) is with providers. We could have a provider with these properties, and use those as defaults for created resources.

const default = new command.Provider("default", {
    enviroment: {
        "HOME": "/Users/ian"
    },
    dir: "/opt/pulumi",
    interpreter: "bash",
});

const command = new command.Command("command", {
    up: "echo $HOME - $PWD - $SHELL",
    interpreter: "zsh"
    // prints /Users/ian - /opt/pulumi - zsh
}, {
    provider: default,
})

Interpreter Signature

I don't understand why this takes an array. Can you elaborate?

[lukehoban]

Great feedback! :tada:

I would prefer something like system (or os) for the package name. I think system.Command is more expressive then command.Command. This is similar to how other langauges model commands, indiciating that the behaviour is platform specific. It also provides a clear place to add other system/os level hooks (like filesystem modeling). I don't feel strongly on this point though.

I see the motivation here - and I see some nice qualities to system.Command, but I'll admit it would not be clear to me what this was about if I saw a package named system in https://www.pulumi.com/registry/. I feel like command is a bit clearly from that perspective?

"command:index:Command" -> "command:local:Command"

I like this a lot, and I do think we can easily do this. In fact, I almost made a change like this as part of the introduction of RemoteCommand. Let's plan on making that change, I'll open a new issue to track that.

I'm concerned that this will make the API noisy.

I agree. I'm inclined to believe that the current API design here is the right thing still on balance, but definitely interested in any other input on this.

Is is possible we could have some kind of "merge and" method?

I don't quite follow how this would address the nested API design question? I could imagine method that combines Command resources, but it feels a little "fancy" relative to normal Pulumi API design. I also couldn't quite tell what scenario this would be needed for.

I don't entirely understand keepers. Could you explain why this would be helpful.

I believe we need to give users the ability to force a replacement of a Command resource when something changes that affects the "identity" of the thing that Command runs on (e.g., when an EC2 instance is replaced, re-run the command). Today, we do this via marking create/delete as forcing replacement, but I believe this is wrong, that changing these should not force replacement (should be an update instead), it should just impact future creations or deletions. (Notably, user can opt-in to making such changes force replacement via replaceOnChanges: ["create"]). But if we don't have those force replacement by default, we do need a way for users to indicate that the Command should be replaced - such as replaceOnChanges: [instance.publicIP]. keepers is a similar concept on Random, but I think calling it something like a property named replaceOnChanges might be clearer? I'm still not certain on the API design here. Might be useful to flesh out some more examples to answer this via practical examples.

If we needed to we could add a input: "alwaysReplace". I would not add this unless it becomes necessary (is requested by users).

Indeed, this is effectively the keepers concept in the previous comment.

pulumi up would run the update hook

This actually raises another good question - do we even need an update hook at all? It's not actually implemented currently, and it's hard to come up with cases where it really makes sense.

One way to make the system more secure is to allow/force exec style arguments. We could provide an interface where each argument is a separate string.

I feel like this would make a lot of examples very complex to write. In fact, there's a related question of whether the input string is a single command, or the contents of a script to process with the interpreter. It will be useful to flesh out some more examples to evaluate this tradeoff. I think strings representing bash scripts are quite common at this level (like userData on EC2 instances), and would prefer to stay closer to that model than the low level os.exec style which is about running a single command, not about running a small script.

I am assuming that the command will fail if a non-zero exit code is returned. Is it work handling non-zero exit codes. I am good with failing on non-zero exit, but I wanted to confirm that it was a conscious decision.

Yes, that is the current design. We might in principle need to allow users to opt for the command to not fail on non-zero exit, but that can currently be accomplished with ping sadasdsa || true.

Another way to model inheritable environmental state (enviroment, dir, interpreter) is with providers.

Interesting idea. I'm not sure this would be particularly common to need to use, but it's something we could definitely add on in the future if there was demand.

[iwahbe]

Is is possible we could have some kind of "merge and" method?

I don't quite follow how this would address the nested API design question? I could imagine method that combines Command resources, but it feels a little "fancy" relative to normal Pulumi API design. I also couldn't quite tell what scenario this would be needed for.

I thought the point of the nested API design was to have a logical resource with different settings (environment, dir, interpreter) for different situations (create, delete, ect.). This would allow for that design goal. Each command would have it's own settings, but we could stitch commands together.

This actually raises another good question - do we even need an update hook at all? It's not actually implemented currently, and it's hard to come up with cases where it really makes sense.

It would be nice to have an update hook that runs when another resource is updated. I don't think an update hook is helpful if it runs when the Command resource is updated.

[lukehoban]

This would allow for that design goal. Each command would have it's own settings, but we could stitch commands together.

Got it. For this scenario, I actually think you can do it already today with just two separate Command resources. They will run either in parallel, or in sequence if onedependsOn the other, and one can be create-only and the other delete-only if that is really important. But even more simply, I think in most cases the commands can be designed to use non-overlapping env vars or the same interpreter, so this should not practically be a big restriction (and is possible to work around in the very rare case that it is a restriction).

It would be nice to have an update hook that runs when another resource is updated. I don't think an update hook is helpful if it runs when the Command resource is updated.

Got it. There is no way in our current model to do the former. The update of the Command resource will store the new create/delete commands into the state file, which in particular will ensure that a future delete uses the updated command.

In theory, there might be use cases for the thing managed by the Command to itself be updated, but I'm not yet sure about what those look like, and they aren't the common cases here, so I'm inclined to hold off on supporting these until we have clear use cases to motivate the API design.

[iwahbe]

@lukehoban I'm glad that clarifies.

For this scenario, I actually think you can do it already today with just two separate Command resources. They will run either in parallel, or in sequence if one dependsOn the other, and one can be create-only and the other delete-only if that is really important.

My solution simplifies this for the user, but it can definitely be modeled with what we already have. If you are not used to .and(T) methods, it does add mental complexity. I'm happy to hold off on special casing this scenario, until and unless it becomes a pain point for users.

[lukehoban]

Closing, as we've moved most of the follow ups here into new issues.