Update SLAS Commands #282

johnboxall · 2022-01-28T22:40:34Z

This is an opinionated fix for #265.

Removed slas:tenant:list. Its backing API requires a staff only / internal role today.
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.
Removed slas:tenant:delete. Its backing API requires a staff only / internal role today.
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.
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:

No way to set required options.
Some options conflict with commander properties (eg. you cannot have a option named --description.
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

This endpoint is only available for internal service users.

- Fix up argument names

-Refactor options - Add help text - Update all client commands - Update API docs - Remove internal APIs

johnboxall · 2022-01-28T22:41:38Z

README.md

@@ -276,7 +276,7 @@ You are now ready to use the tool by running the main command `sfcc-ci`.

 Use `sfcc-ci --help` or just `sfcc-ci` to get started and see the full list of commands available:

-```bash
+```txt


Fixes highlighting issues:

johnboxall · 2022-01-28T22:42:56Z

README.md

@@ -978,121 +977,84 @@ callback         | (Function)  | Callback function executed as a result. The err

 APIs available in `require('sfcc').slas`:

-`tenant.add(tenantId, shortcode, description, merchantName, contact, emailAddress, fileName)`
+`tenant.add({shortcode, tenant, file})`


These many argument style interfaces are error prone and very hard to change in a backwards compatible way.

I've moved all the interfaces to accepting objects with named attributes.

I understand. Please see the dicusssion for the original intent and the suggestion in last line of combining best of both worlds

hnestmann · 2022-01-31T09:03:57Z

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

hnestmann · 2022-01-31T09:10:30Z

lib/slas.js

@@ -1,18 +1,22 @@
 const fetch = require('node-fetch');


Thanks very useful changes in that module. Appreciate you fixing my sloppy copy and paste errors

johnboxall · 2022-01-31T21:21:24Z

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.
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.
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 · 2022-02-01T08:59:08Z

@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:

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.
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 · 2022-02-01T15:14:55Z

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!

- Also throw an exception if the token available isn't a JWT. - And prettier format just to make a giant diff

johnboxall · 2022-02-04T05:43:42Z

@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 · 2022-02-04T08:27:16Z

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

@johnboxall Awesome, thanks a lot!

tobiaslohr · 2022-02-15T12:29:05Z

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 · 2022-02-15T13:02:58Z

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 #283

tobiaslohr · 2022-02-15T13:07:35Z

README.md

-    slas:client:delete [options]                                    Deletes a SLAS client from a given tenant
-
-  Environment:
+    slas:tenant:add [options]                                       Add or update SLAS a tenant.


As described in #282 (comment), I'd suggest to rename to :create, instead of :add, to follow existing patterns.

tobiaslohr · 2022-02-15T13:08:44Z

README.md

-    slas:client:delete [options]                                    Deletes a SLAS client from a given tenant
-
-  Environment:
+    slas:tenant:add [options]                                       Add or update SLAS a tenant.


As described in #282 (comment), I'd suggest to leave slas:tenant:list

tobiaslohr · 2022-02-15T13:10:43Z

README.md

-
-  Environment:
+    slas:tenant:add [options]                                       Add or update SLAS a tenant.
+    slas:tenant:get [options]                                       Get a SLAS tenant.


As described in #282 (comment), I'd suggest to use slas:tenant:list --tenant <tenant> instead of sfcc-ci slas:tenant:get --tenant <tenant> to retrieve details of a single tenant.

tobiaslohr · 2022-02-15T13:11:11Z

README.md

-  Environment:
+    slas:tenant:add [options]                                       Add or update SLAS a tenant.
+    slas:tenant:get [options]                                       Get a SLAS tenant.
+    slas:client:add [options]                                       Add or update a SLAS client for a tenant


As described in #282 (comment), I'd suggest to rename to :create, instead of :add, to follow existing patterns.

tobiaslohr · 2022-02-15T13:12:29Z

README.md

+    slas:tenant:add [options]                                       Add or update SLAS a tenant.
+    slas:tenant:get [options]                                       Get a SLAS tenant.
+    slas:client:add [options]                                       Add or update a SLAS client for a tenant
+    slas:client:get [options]                                       Get a SLAS client for a tenant.


As described in #282 (comment), I'd suggest to use slas:client:list --tenant <tenant> --clientid <clientid> instead of sfcc-ci slas:client:get --tenant <tenant> --clientid <clientid> to retrieve details of a single client.

tobiaslohr

Consider changes suggested in #282 (comment) and #282 (comment)

johnboxall · 2022-02-16T06:29:08Z

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.

… fix-slas

johnboxall added 4 commits January 27, 2022 15:54

Remove slas:tenant:list

3fe366f

This endpoint is only available for internal service users.

Fix slas:tenant:add

dd73e32

- Fix up argument names

WIP – client list and get

c357f15

Update SLAS integration

40c9de7

-Refactor options - Add help text - Update all client commands - Update API docs - Remove internal APIs

johnboxall commented Jan 28, 2022

View reviewed changes

johnboxall added 2 commits January 28, 2022 14:57

Fix linting issues

1dc5cdc

Really fix linting.

cbe226f

hnestmann reviewed Jan 31, 2022

View reviewed changes

johnboxall mentioned this pull request Feb 2, 2022

[BUG] - Error during login - Invalid channel_id SalesforceCommerceCloud/pwa-kit#376

Closed

johnboxall added 4 commits February 3, 2022 15:43

Adds SLAS unittests

4db82e7

- Also throw an exception if the token available isn't a JWT. - And prettier format just to make a giant diff

Merge branch 'master' into fix-slas

712de9c

Test all the things

8a662ae

Hopefully fix weird test.

3ffff8f

tobiaslohr added 2 commits February 11, 2022 17:34

Merge branch 'master' into fix-slas

b963a28

Merge branch 'master' into fix-slas

3d8bcb7

tobiaslohr added api-change Change that will most likely break existing APIs and may be rolled out into a new major version enhancement New feature or request labels Feb 15, 2022

tobiaslohr added this to the 3.0.x milestone Feb 15, 2022

tobiaslohr reviewed Feb 15, 2022

View reviewed changes

tobiaslohr requested changes Feb 15, 2022

View reviewed changes

johnboxall added 5 commits February 25, 2022 14:37

Add credential quality command

611f21f

Merge branch 'fix-slas' of https://github.com/johnboxall/sfcc-ci into…

4e31d33

… fix-slas

Rename SLAS commands add -> create

e7c6539

Consolidate client list command

7e5bd60

...

2a9473c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SLAS Commands #282

Update SLAS Commands #282

johnboxall commented Jan 28, 2022 •

edited

johnboxall Jan 28, 2022

johnboxall Jan 28, 2022

hnestmann Jan 31, 2022

hnestmann commented Jan 31, 2022 •

edited

hnestmann Jan 31, 2022

johnboxall commented Jan 31, 2022

tobiaslohr commented Feb 1, 2022

johnboxall commented Feb 1, 2022

johnboxall commented Feb 4, 2022

tobiaslohr commented Feb 4, 2022

tobiaslohr commented Feb 15, 2022

tobiaslohr commented Feb 15, 2022

tobiaslohr Feb 15, 2022

tobiaslohr Feb 15, 2022 •

edited

tobiaslohr Feb 15, 2022

tobiaslohr Feb 15, 2022

tobiaslohr Feb 15, 2022

tobiaslohr left a comment

johnboxall commented Feb 16, 2022 •

edited

Update SLAS Commands #282

Are you sure you want to change the base?

Update SLAS Commands #282

Conversation

johnboxall commented Jan 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hnestmann commented Jan 31, 2022 • edited

Choose a reason for hiding this comment

johnboxall commented Jan 31, 2022

tobiaslohr commented Feb 1, 2022

johnboxall commented Feb 1, 2022

johnboxall commented Feb 4, 2022

tobiaslohr commented Feb 4, 2022

tobiaslohr commented Feb 15, 2022

tobiaslohr commented Feb 15, 2022

Choose a reason for hiding this comment

tobiaslohr Feb 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiaslohr left a comment

Choose a reason for hiding this comment

johnboxall commented Feb 16, 2022 • edited

johnboxall commented Jan 28, 2022 •

edited

hnestmann commented Jan 31, 2022 •

edited

tobiaslohr Feb 15, 2022 •

edited

johnboxall commented Feb 16, 2022 •

edited