Update SLAS Commands

[johnboxall]

This is an opinionated fix for https://github.com/SalesforceCommerceCloud/sfcc-ci/issues/265.

1. Removed slas:tenant:list. Its backing API requires a staff only / internal role today.
2. Reworked the options for slas:tenant:add. Generally I removed options that I felt would be frustrating to set on the command line. Instead, I'd like to encourage folks to use the --file based interface. I also tried to code this such that you could provide only a tenant and it would work.
3. Removed slas:tenant:delete. Its backing API requires a staff only / internal role today.
4. Reworked options for slas:client:add. I think setting client options via command line is setting yourself up to fail. Instead, we force folks to pass a --file.
5. Removed table display options from slas:client:(get|list) – there is no way to display a SLAS client (or clients!) as a table.

Personally, I'd vote for making all these changes, as I think the smaller interface surface gives us lots of room to grow.

---

While making these changes I ran into a # of limitations of Commander@2:

1. No way to set required options.
2. Some options conflict with commander properties (eg. you cannot have a option named --description.
3. No way to accept a value of - for an option (make a normal pattern of saying --file - and accepting STDIN impossible)

Generally where possible, I've tried to follow the existing patterns in the repo.

❤️ ❤️ ❤️

cc @hnestmann & @tobiaslohr

[hnestmann]

Thanks John for the contribution. Really appreciate your work here:

This is an opinionated fix for #265.

1. Removed `slas:tenant:list`. Its backing API requires a staff only / internal role today.

To be quite honest - I don't think we should remove it. It is a documented production API, that has to work. The CLI usability is questionable, but for JS APIs dealing with the implicite dependency between on-demand-sandboxes and slas, it is be useful https://developer.salesforce.com/docs/commerce/commerce-api/references#shopper-login-and-api-access-admin:retrieveTenants

2. Reworked the options for `slas:tenant:add`. Generally I removed options that I felt would be frustrating to set on the command line. Instead, I'd like to encourage folks to use the `--file` based interface. I also tried to code this such that you could provide only a `tenant` and it would work.

I wouldn't do it in commandline either, but I thought it might be useful if you build CI tools with python or bash scripts

3. Removed `slas:tenant:delete`. Its backing API requires a staff only / internal role today.

See 1) https://developer.salesforce.com/docs/commerce/commerce-api/references#shopper-login-and-api-access-admin:deleteTenant

4. Reworked options for `slas:client:add`. I think setting client options via command line is setting yourself up to fail. Instead, we force folks to pass a `--file`.

See 2

5. Removed table display options from `slas:client:(get|list)` – there is no way to display a SLAS client (or clients!) as a table.

Yep, that's indeed not working. I will see if we can flatten the info to fit in a table. But for now we can remove it

Personally, I'd vote for making all these changes, as I think the smaller interface surface gives us lots of room to grow. I am open for the syntactical changes. I am also open to hide the (currently) not working APIs. But I prefer to push for a fully working API Set.

WRT to --file. I think @tobiaslohr had an interesting comment on the original PR. We can pipe a file into the command vs -- file

sfcc-ci slas:client:add < my_file.txt or sfcc-ci slas:client:add {"clientID":"123"} would be equivalent, combining the efecitiveness of a file with the flexibility of just issueing a one line command

[johnboxall]

1. Regarding slas:tenant:list – I agree that this would be useful to have! But the backing API doesn't support it. Right now, the tenant list API endpoint is staff only. Until that is not the case, I suggest we leave it out. (eg. I'm advocating for adding it in, once it is supported.
2. Regarding slas.client:add – It seems like we're aligned. I'd say ship the file/stdin based API, gather feedback and iterate from there.
3. Regarding slas:tenant:delete – Again, this endpoint today only supports staff user requests. I've removed it for now. Once support for end users calling it lands, lets add it back!

I'll make an update for add the stdin based file input to client/tenant add.

---

@hnestmann / @tobiaslohr – one question ... to operate sfcc-ci requires folks set SFCC_OAUTH_CLIENT_ID. With SLAS, your AM Client must have its Access Token Format set to JWT (as opposed to UUID).

I think its very easy to trip into this and not have your AM Client setup right. I also imagine this means you need at least two AM clients, as I'd imagine some of the old APIs not accepting JWTs.

Have we considered "shipping" a API Client with the App? eg. including a set of well known API Client values that are configured correctly for use with sfcc-ci? From my perspective, there are no security implications from doing this (?)

[tobiaslohr]

@johnboxall Thanks a lot for your contribution! Very much appreciated!!

Have we considered "shipping" a API Client with the App? eg. including a set of well known API Client values that are configured correctly for use with sfcc-ci? From my perspective, there are no security implications from doing this (?)

Thanks for raising this. Yes, we've considered this already and we've also discussed this with the Salesforce Product Team. I see 2 main challenges with this:

1. The current client passed for running the interactive authentication using sfcc-ci auth:login <client> is also used to run commands (and author API calls to ecom APIs aka OCAPI) that being said. Assuming you allow that API client access to your resources on an ecom instance you would allow anyone running the commands as the permission is bound to the API client and not the user.
2. ecom does not support JWT based access token formats for oauth grant types (like implicit, implemented by sfcc-ci auth:login). A public client shipped with the CLI however we should be set with the JWT token format.

Thoughts? I suggest we open a separate issue for this to have a forum to discuss.

[johnboxall]

Good points @tobiaslohr – I think it is possible to work around both of these concerns. I agree, let's ship that discussion to a separate issue.

With respect to this PR, before I merge, we also need to resolve an issue with node-fetch – in a separate PR (#270), we updated node-fetch@3. Version 3 is incompatible with the current codebase, and this wasn't caught by tests.

I'm going to attempt to swap the code over to the existing requests dependancy, remove node-fetch and add at least one test that would catch this sorta regression going forward.

Thanks!

[johnboxall]

@tobiaslohr / @hnestmann I added unit tests. They're not the best, but significantly better than nothing!

I think there is a more you could improve here (eg. adding stdin handling for tenants/clients) but if its okay, I'd love to merge this incremental improvement which fixes blocking bugs in these commands.

Let me know if there is any feedback you would like addressed. 🙌

[tobiaslohr]

I added unit tests. They're not the best, but significantly better than nothing!

@johnboxall Awesome, thanks a lot!

[tobiaslohr]

General feedback:

• We should follow the existing patterns for commands, incl. CRUD operations. Therefore for the creation of the new object (like a slas tenant or a client) we should be using slas:tenant:create instead of slas:tenant:add and slas:client:create instead of slas:client:add
• Due to the above, we will need to put this into a new major version, as it would be a breaking change. Same with commands we remove etc.
• Commands to get details on a tenant or client should be using the respective list command with the object id passed in, e.g. we should be using slas:tenant:list --tenant <tenant> instead of slas:tenant:get --tenant <tenant> and slas:client:list --clientid <clientid> --tenant <tenant> instead of slas:client:get --clientid <clientid> --tenant <tenant> (they would use the GET method on the respective API endpoints to retrieve a single object, not the GET on the list of objects).
• On the permission discussion and certain APIs we adopt requiring a staff only role: This is reality today for other commands as well, e.g. org:list. All APIs itself must implement proper authentication and authorization to ensure only authenticated and authorized users can perform operations. The CLI is used by SFDC staff as well, so I see no issue exposing commands where the adopted API requires a staff only role.

[tobiaslohr]

On the discussion regarding reading the object details as JSON (e.g. inline or stored in a file) vs. passing the object details as multiple arguments to the command I feel that we should support both. It may be error prone to pass the properties as multiple arguments as you outlined @johnboxall, however as we support this with other commands and there is an alternative, you always have the choice.

There is even a path with a hybrid approach (which is partially implemented by sfcc-ci user:create respectively), where explicit properties (in case passed) are being merged into the object properties given through the JSON (see https://github.com/SalesforceCommerceCloud/sfcc-ci/blob/master/lib/user.js#L268).

On top there even are ideas for setup commands and provide a wizard like step by step process via the CLI, which would be most suited for less advanced users, see https://github.com/SalesforceCommerceCloud/sfcc-ci/issues/283

[johnboxall]

• We should follow the existing patterns for commands, incl. CRUD operations. Therefore for the creation of the new object (like a slas tenant or a client) we should be using slas:tenant:create instead of slas:tenant:add and slas:client:create instead of slas:client:add

Makes sense!

• Due to the above, we will need to put this into a new major version, as it would be a breaking change. Same with commands we remove etc.

Yup.

• Commands to get details on a tenant or client should be using the respective list command with the object id passed in, e.g. we should be using slas:tenant:list --tenant <tenant> instead of slas:tenant:get --tenant <tenant> and slas:client:list --clientid <clientid> --tenant <tenant> instead of slas:client:get --clientid <clientid> --tenant <tenant> (they would use the GET method on the respective API endpoints to retrieve a single object, not the GET on the list of objects).

Make sense.

• On the permission discussion and certain APIs we adopt requiring a staff only role: This is reality today for other commands as well, e.g. org:list. All APIs itself must implement proper authentication and authorization to ensure only authenticated and authorized users can perform operations. The CLI is used by SFDC staff as well, so I see no issue exposing commands where the adopted API requires a staff only role.

I'm not sure I agree with this design decision, but that doesn't matter. If its an existing pattern, sure lets add them back, but I have no way to validate the output as I don't personally have the required super duper staff permission.

On the discussion regarding reading the object details as JSON (e.g. inline or stored in a file) vs. passing the object details as multiple arguments to the command I feel that we should support both. It may be error prone to pass the properties as multiple arguments as you outlined @johnboxall, however as we support this with other commands and there is an alternative, you always have the choice.

That's fine, I'd only mention that the SLAS client object has been accreting fields lately, and we'd potentially setting ourselves up for a bunch of iterative work over time adding all the properties.

As discussed in Slack, I'm 100% (maybe even 110%) a-okay with you driving the implementation of the changes you feel are required.