fortify / fcli

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

FoD: Refactor argument options to lowercase #329

Closed kadraman closed 4 months ago

kadraman commented 1 year ago

For consistency with other modules any FoD argument options should be changed to lowercase. For example:

fod app create-microservice-app --criticality=High --status=Development --owner=kevin.lee --microservices=ms1,ms2,ms3 --release-microservice=ms2 --auto-required-attrs fcli-test-a:1.0

should be changed to allow:

fod app create-microservice-app --criticality=high --status=development --owner=kevin.lee --microservices=ms1,ms2,ms3 --release-microservice=ms2 --auto-required-attrs fcli-test-a:1.0

I believe the API is case insensitive but all commands will need to be tested and test scripts updated.

rsenden commented 1 year ago

I agree that this would make the FoD module more consistent with other fcli modules. However, what about consistency with for example the -q option?

For example, based on your proposal, I guess we could end up with a situation like the following, where lowercase high needs to be used in the create command, but pascal-case High needs to be used in queries (using lowercase high in the query wouldn't return anything):

fod app create-microservice-app --criticality=high
fcli app list -q 'criticality=="High"'
kadraman commented 1 year ago

Yes, agreed but see below...

With your example, to get it to work in FoD you would actually need to do:

fcli fod app list -q "businessCriticalityType=='High'"

(powershell you need to swap single and double quotes)

even more confusing to query on the type, if you wanted to see web apps you would need to use the internal value:

fcli fod app list -q "applicationType=='Web_Thick_Client'"

As you know all the -q options can be found using -o json-properties but it does require knowledge of the FoD API to really understand what the values are.

Parking this one until we can or cannot come up with a better way.

rsenden commented 1 year ago

Potentially we could use record transformers to transform both property names and values, for example transform "applicationType": "Web_Thick_Client" to "type": "web". This would allow users to do fcli fod app list -q 'type=="web"'.

However:

rsenden commented 1 year ago

As suggested in internal email, for ease of use and consistency with other modules, we'll probably want to use lower-case option values. Querying is a relatively advanced topic, especially given the comment from @kadraman above, so differences in case are probably the least of our worries.

In order to change current PascalCase option values to kebab-case values, enum values representing those option values should be changed to lowercase. For multi-word values, an _ will need to be inserted between each word, and ITypeConverter and default value provider will need to be configured on the appropriate options to convert _ to - and vice versa. See ProgressWriterTypeConverter and corresponding option configuration for the --progress option in ProgressWriterFactoryMixin. Ideally, at some point we should be able to handle such conversions in a generic way; see #306.

Obviously, this also depends on whether we have predefined enums listing allowed values for each option, or whether users would need to look up option values from FoD (see #361). If the latter, we'd either need to use the original FoD lookup values, or have the lookup command output both original PascalCase FoD lookup values (for use in queries, rest call command, ...), and those values converted to kebab-case (for use as option values).

rsenden commented 1 year ago

After closer inspection, this will likely require too much work to implement and maintain, and may reduce user experience due to inconsistencies with query/output expressions, so most likely we'll just stick to whatever format is used by the target system.

rsenden commented 4 months ago

Closing this issue, as we've long made the decision to stick to whatever format is used by the target system.