PowerShell / Modules

MIT License
112 stars 25 forks source link

[SecretsManagment][Feature] Add-Secret should be named Set-Secret #37

Closed Jaykul closed 4 years ago

Jaykul commented 4 years ago

In the interface for the Secret Vaults, we have Set-Secret as the verb, and the vault is expected to automatically overwrite any secret that already existed with that name.

But in the public module commands, you've incorrectly used the verb Add with a very unintuitive NoClobber switch. This is confusing for users, but even more confusing for anyone who's studied the RFC or the vault design and seen Set-Secret all over the place.

We need consistent use of verbs

It's not just PowerShell, but in programming in general, the Add verb is used for non-destructive operations where you are appending to a collection without overwriting anything that's already in the collection.

In PowerShell the built-in Add commands are implemented this way, and in the rare occasion where they have the ability to overwrite, it must be explicitly invoked with a -Force switch.

In fact, that's usually also true for the New verb, for example:

When we want to overwrite, or to change the value of something, we use the verb Set:

As an additional point: the -NoClobber parameter, although common, has never been used on an Add command before now. It's usually used only with verbs that implicitly create things, such as Export and Write

And further, the Add-Secret command is implemented by literally invoking set-secret!, and the comment on the line above it is incorrect because it's not necessarily a new secret (unless the user specified -NoClobber to make the command behave the way an Add command should behave).

Proposed technical implementation details

Please rename AddSecretCommand command to SetSecretCommand and change the verb to Set.

Alternatively: change the NoClobber parameter to Force and invert the logic so that it defaults to not clobbering, as users will expect.

JustinGrote commented 4 years ago

Related to #30, we want to uphold the "Powershell Promise" that people won't have to relean Powershell once they learn it, so the conventions in this command should behave as similar as possible to common commands users are familiar to like the *-Item commands.

If you choose Set-Secret, it should overwrite by default. If you choose New-Secret, it should error if a duplicate exists unless -force is specified. Of course both can be specified and just have Set-Secret call New-Secret -Force

Jaykul commented 4 years ago

@JustinGrote I agree, and that's what I meant. The current command (which overwrites by default) is implemented as Set and would be ok if it was named that.

Although I have to say, if I were creating this whole thing, I would have implemented is as a PSProvider...

JustinGrote commented 4 years ago

@Jaykul we can still do that with SHIPS :). One folder per provider, then the secrets under there.

iSazonov commented 4 years ago

/cc @mklement0 for information.

jeremymcgee73 commented 4 years ago

I want to add that the example module does not actually overwrite. It throws an error if the secret exists. That is what my module currently does as well.

Jaykul commented 4 years ago

The "example"? The actual default provider does not do that. Besides, if you throw an error when the secret exists, how can you update a secret @jeremymcgee73?

SydneyhSmith commented 4 years ago

Thanks for all the feedback, closing as a result of PR #51