Repository navigation
Client API v2 CLI client: rename update command to apply to reflect that it can create resources as well as update them - #48134
Conversation
ada1944 to
2a5bd3e
Compare
|
@michalvavrik first of all, thanks for another CLI improvement! I know that discarding UUID silently on create-via-PUT was a design choice, discussed many times, so certainly I don't want to re-open it. I just really don't like the CLI UX consequence of it, especially now, when I have it just in front of my eyes: works, returns the real server-generated UUID. Naturally then, the following also works: And then the one I started with fails, although expectedly: You mentioned So rather a question here - should we do something about it as well? |
The only other behavior we're discussing is to disallow the uuid from being present when performing a create with a PUT. That would change the first apply in your scenario to a failure. This also matches with kubernetes server side apply. In a similar situation you would see an error like:
|
Just seconds before you Steve posted the reaction, I re-formulated my last sentence, as I was less and less sure what and how it should be done. 😊 Thanks for the info, it may be a good alternative or it's just a knee-jerk reaction from me and all I need is to use to the UX after all. |
@Pepo48 & @shawkins I was explicitly told by @vmuzikar before I started that supplying IMO we shouldn't do client-side validation of presence of If you feel strongly about it, I don't mind dropping |
|
To be clear, the change in behavior I'm talking about would be on the server side, not the tooling. |
In that case I am +1, but as far as I understand the situation, there wasn't agreement on such change on Wednesday. I can create a new sub-task outside of the MVP task group, e.g. under Client v2 enhancements #47822, so that we can revisit it? |
|
I'd remove
That IMHO makes sense mostly only for file input – I fetch the client and store it to file, make some modifications, and PUT it back to the server. Then the validation fails if the resource was renamed. |
2a5bd3e to
52ccc50
Compare
done, thanks for moving this discussion forward |
* Closes: keycloak#47472 * Problem with 'update' command is that it is misleading, therefore the command we generate for 'PUT' HTTP method, which either creates or updates a resource, is now called 'apply'. * The '--uuid' option is removed from commands Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>
52ccc50 to
87f0fd1
Compare
I agree with removing the parameter. About the only thing useful to do with something like that would be to unset the field. We'll just need to rely on users understanding the purpose of the UUID and the clarity of server side error messages. Rest assured someone will start off by fetching a resource from a different server, then try to apply the file twice, and see the the second apply. |
I wanted to discuss this today on the WG meeting but there wasn't time. I am not sure if we should do that. One thing that I found out yesterday when working on the |
I think the general plan is to tackle renames later, if at all. There's a lot of details to straighten out about the renaming operation - some stuff we're associating by name, other it's by uuid. So for now for client admin api v2 we're just assuming that users will delete and recreate. |
There was a problem hiding this comment.
LGTM, thanks @michalvavrik
* Closes: keycloak#47472 * Problem with 'update' command is that it is misleading, therefore the command we generate for 'PUT' HTTP method, which either creates or updates a resource, is now called 'apply'. * The '--uuid' option is removed from commands Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>
updatesub-command to reflect the fact it either create or update client #47472updatecommand is that it is misleading, therefore the command we generate for thePUTHTTP method, which either creates or updates a resource, is now called 'apply'.--uuidoption and verifying that we print clear validation failure when user specified a wrong UUID, so I added it as it can happen.--uuidoption is removed fromcreatecommand as it is ignored and users could be pretty confused if we allow them to specify--uuidwhich is later ignored. On the other hand, I kept it forpatchandapplycommands as there, users can use it as additional measure to ensure they are updating the same client (if someone had renamed the clients (e.g.A -> B & C -> A), then UUID check would fail