fortify / fcli

fcli is a command-line utility for interacting with various Fortify products
https://fortify.github.io/fcli/
Other
29 stars 16 forks source link

Review `fcli fod app create-*-app` commands #367

Closed rsenden closed 12 months ago

rsenden commented 1 year ago

These commands mimic multi-page wizards in the FoD UI, resulting in many command-line options and causing these commands to be not very intuitive to use.

Any way we can improve the structure of these commands and options, maybe modelling things a bit different than the FoD UI but more suitable for CLI? Since apps apparently can't live on their own without release/micro-services, maybe we shouldn't have these create-*-app commands in the first place, but instead:

Some particular observations:

kadraman commented 1 year ago

We had quite a long email conversation on this a while ago and separate create-XXX-app is the best we came up with at the time?

I don't like driving everything of the release as this is not what happens in the UI and given that there were more specific options for each app type, you would end up with specifying fod release create-web-app, fod release create-mobile-app etc . I think this is worse and even clunkier when adding a new release to an app :confused:

No users (other than the owner) are assigned to the release at creation time as this is what the UI does as well.

Other observations are all fine and will look at implementing.

rsenden commented 12 months ago

Indeed we did have some discussions about the best approach, but while reviewing the current command structure, I started wondering whether this can be further improved. Some considerations:

Maybe we should have something like the following instead:

fcli fod app create[-with-releases] <app-name> --type Mobile|Web|ThickClient --releases r1,r2
fcli fod app create-with-microservices <app-name> --releases ms1:r1,ms1:r2,ms2:r1

If users want to set additional microservice/release properties like release description, they can use the fcli fod release update command after creating the app. Alternatively, we could also consider a syntax like the following, but probably not a good idea: --releases 'r1:Some description,r2:Other description'

I don't particularly like the long create-with-microservices command though (or the create-microservices-app alternative), hence the earlier idea to have something like the following instead:

fcli fod release create-app <app-name> --type Mobile|Web|ThickClient --releases r1,r2
fcli fod microservice create-app <app-name> --releases ms1:r1,ms1:r2,ms2:r1

By not replicating exact FoD UI functionality, all of the commands above now look pretty similar so we could even consider combining everything in a single command again, accepting either plain releases or releases scoped by microservice:

fcli fod app create <app-name> --type Mobile|Web|ThickClient --releases r1,r2
fcli fod app create <app-name>--releases ms1:r1,ms1:r2,ms2:r1

Not sure whether this is a good idea though as --type is not relevant for microservice apps, and the command would need some conditional logic based on whether releases are specified with or without micro services.

What do you think?

kadraman commented 12 months ago

I'm not happy with the current incarnation, but some of the examples mentioned are more or less what we had previously.

Technically, this makes sense:

fcli fod release create-app <app-name> --type Mobile|Web|ThickClient --releases r1,r2

but doesn't feel right creating an app of a release command from a users perspective.

I do actually like this one:

fcli fod app create <app-name> --type Web|Mobile --releases r1,r2
fcli fod app create-with-microservices <app-name> --microservices ms1,ms2 --releases ms1:r1,ms1:r2,ms2:r1

although not sure about creating multiple releases as we would then have to potentially specify the "description" and "sdlc status" that is required for each one, e.g.

fcli fod app create <app-name> --type Web --releases r1,r2 --sdlc-status r1:Development,r2:Production --release-description "release 1","release2" ...

Which is messy? If we limit to one release being creating then this works for me:

fcli fod app create <app-name> --type Web|Mobile --release r1 --release-description "release 1" --sdlc-status "Development"
fcli fod app create-with-microservices <app-name> --microservices ms1,ms2 --release ms1:r1 --release-description "release 1" --sdlc-status "Development"

Is this OK?

Then we have how to creating additional releases which currently uses <app-name>:<release-name> as a parameter:

fcli fod release create ... --microservice ms2 .... <app-name>:<release-name>
fcli fod release create --app <app-name> --name r1 --description "release 1" --sdlc-status "Development"
fcli fod release create-on-microservice --app <app-name> --microservice ms2 --name r2 --description "release 1" --sdlc-status "Development"

Any good?

rsenden commented 12 months ago

I didn't realize that there were more options than just --release-description for configuring the release, so indeed, then it's better to have fcli app create* create only a single release.

If we'd deviate from the FoD UI to allow only a single microservice to be created when creating the app, we could potentially even merge everything in a single command again as we'd no longer need the microservice-specific options (having options like --microservices and --release-microservice was the primary reason for having separate create-* commands):

fcli fod app create <app-name> --type|-t Web|ThickClient|Mobile|Microservice --release [ms1:]r1 --release-description ...

If type==Microservice, the release must be specified as ms1:r1, for other types it must be specified as just r1. All other options are the same, independent of what type of application you're creating. What do you think about this?

Also not sure how to best represent Web/Thick Client in fcli; maybe we should have both as separate types (but mapping to the same combined value being sent to FoD), or maybe have something like --type|-t Regular|Mobile[|Microservice].

As for fcli fod release create, for consistency with for example fcli ssc appversion create, I'd prefer to pass app:[ms:]:release as a positional parameter, either with a single create command that takes both ms and non-ms formats, or with two separate create commands like you proposed. So either the following two commands, or both combined in a single fcli fod release create command:

fcli fod release create app:release --description ... --sdlc-status ...
fcli fod release create-on-microservice app:ms:release --description ... -- sdlc-status ...

Related question with regards to creating microservice releases; should this:

kadraman commented 12 months ago

I think when creating a microservice app we still need to be able to create multiple microservices as this is how the creator (maybe Security Lead) will envision it at the time (or be requested to create it). Releases (other than the first) will probably be created by other users (Developers) as and when needed.

Also, when creating an application the app name and release name are specified as PositionalParameter mixin: fcli fod app create <app-name>:<release-name> ... - I don't really like this but it is consistent with other operations - do we want to keep this or specify --release option like I had previously?

If we do, how about:

fcli fod app create app:release --type web|thick-client|mobile ...
fcli fod app create-with-microservices app:release --microservices m1,m2,m3 ...

web and thick-client are mapped to the same FoD type?

and

fcli fod release create  <app:release> --status ...
fcli fod release create-on-microservice <app:ms:release> --status ...
rsenden commented 12 months ago

App creation & micro services Even if we only allow a single microservice to be created on the app create command, you could still immediately follow that by one or more microservice create commands to implement whatever the creator/security lead envisions, right? It's just a slightly different set of commands but with the same outcome.

It does however offer much more consistency between creating regular apps and microservice apps, and does potentially allow us to have a single command for creating any type of application (which makes it consistent with the single 'Create Application' button in FoD).

Positional parameters As for the positional parameters, these should always represent the entity that's being operated upon, and should be consistent with other commands in the same entity. So, for fcli fod app * commands, the positional parameter should be just the app name (or app id when referencing existing apps); anything else like release or micro service names should go into options. Or, in other words, positional parameter in fcli fod app create should represent the same entity as in fcli fod app get.

Similarly, for fcli fod release * commands, positional parameters should represent the fully qualified release name (or id when referencing existing releases), so either app:release or app:ms:release.

kadraman commented 12 months ago

So, with the exception of specifying release with --release this is exactly what we had initially (see screenshot).

fcli-fod-app-create

We changed it because there was too much conditional logic for determining the correct options when microservices app was being created.

ms-logic

and it was getting confusing for the user with the large number of (not necessarily required) options.

Microservices is an optional feature and is not enabled on the tenant unless the customer has requested them

Given the above, I'm happy to put it back as was but I'm also happy that there is a specific command for this "optional" feature.

What do you suggest? 😆

I'll do --release anyway as it doesn't match and if we are only creating one microservice the user won't have to specify the --release-microservice option (i.e. which microservice to create the release on) - although if they don't specify it I assume first microservice anyway.

rsenden commented 12 months ago

I think the main concern with the original approach was having multiple options that were only relevant when creating a microservice app; --microservice to create one or more microservices, and --release-microservice to specify which of those microservices to create the release on. In particular the latter option was difficult to comprehend for inexperienced FoD users.

Allowing only a single microservice to be created greatly simplifies this; only a single microservice name needs to be accepted by the create command, and no need for an option to specify on which microservice to create the release.

As suggested, the microservice to be created could be specified in the -release option as ms1:r1, or alternatively I think having a separate --microservice option is also still acceptable, as the option name directly corresponds to --type=Microservice.

If only we'd realized this earlier, we wouldn't have need to go through all these intermediate discussions and implementations ;)

kadraman commented 12 months ago

Ok, I think I've got it now, we'll go back to single fod app create, I'' add --release option and I'll keep --microservice:

fcli fod create app MyApp -type Microservice --criticality High ... --microservice ms1 --release r1 --status Development
fcli fod create app MyApp -type Web --criticality High ... --release r1 --status Development
...

What do you think about defaulting "criticality" and "status" (release), I already have --auto-required-attrs

I'll see how much I can get done by tomorrow.