search

fortify / fcli

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

artifact download vs download-by-id #338

Closed xakrurychle closed 1 year ago

xakrurychle commented 1 year ago

diff <(./fcli ssc appversion-artifact download-by-id -h) <(./fcli ssc appversion-artifact download -h) There is no difference in these two options which makes one of these redundant. Both options only accept artifactID either way.

fcli version 0.20230609.101848-dev_2.0.0-beta, built on 2023-06-09 10:19:16

rsenden commented 1 year ago

@xakrurychle Thanks for reporting; if you look at the output of ./fcli ssc appversion-artifact -h, you'll see that download-by-id and download are just aliases for the same command. The reason for having the download-by-id alias is to differentiate this command from the download-state command.

Maybe we should remove the download alias though; if you see the download command in a pipeline for example, you'd need to check the help output to understand whether this downloads an individual artifact (by id) or the application state.

Alternatively, maybe we should just have a single fcli ssc appversion-artifact download command that downloads an artifact by id, and move the download-state command to fcli appversion download-state, which seems a bit more logical as you're downloading current state of an application version, not the current state of an artifact (current state just happens to be an artifact). Main drawback is that this is not consistent with the SSC web UI, as both download-by-id and download-state functionalities are on the Artifacts page.

@xakrurychle @wtfacoconut @kadraman @Andhrei Any preference on how we should structure this?

MikeTheSnowman commented 1 year ago

Hey @rsenden. There are two possible options that I support:

  1. Removing the download-by-id alias and providing the download-state command with an improved description to make it more clear what the difference is between the two commands. As an example description for download-state: Download the latest fully merged result set of an application version. Maybe that description isn't the best, but that's just one possibility.
  2. The other idea is that we just have a single download that returns the latest "current state" artifact from an app-version. Alternatively if the user wants to download a specific artifact from the app-version, then they can supply the artifact id with the --artifact-id option.
    • I believe that most users, most of the time, will be more interested in getting the latest state from an app-appversion, which is why I think we can probably afford to drop the dedicated command to download a specific artifact-id.
    • One less command for us to maintain
    • Example command (download latest state): fcli ssc appversion-artifact download app1:ver7
    • Example command (download specific artifact): fcli ssc appversion-artifact download app1:ver7 --artifact-id 1234
rsenden commented 1 year ago

@wtfacoconut

  1. I think having download and download-state commands within a single entity might be confusing for users as to what is being downloaded by the non-qualified download command. For this reason, I proposed to either just have download-by-id and download-state commands, with behavior being easy to guess from command names, or moving the download-state command to appversion download-state, thereby having appversion-artifact operating on individual artifacts only and being more consistent with other entities; the download command would operate on individual artifacts/id's, similar to the get command for example.

  2. I think we used to have a single download command, but we explicitly split this into separate commands for consistency/maintenance/atomic-command reasons. In particular, the download-by-id command allows the artifact id to be passed as a positional parameter, thereby being more consistent with for example the appversion-artifact get command. We wouldn't have that consistency if we had a download command that supports downloading both individual artifact by id and application state.

Can you please elaborate on why you didn't like the ideas provided in my previous comment? To summarize, my idea was to have either:

rsenden commented 1 year ago

As both download-state and purge-older-than commands operate at application version level, rather than individual artifact level (and as a consequence output an application version object rather than artifact object), they don't fit very well with the other artifact-related commands. As such:

So, the entity-specific commands now have a simple, short name consistent with other modules/command trees. Usage instructions for appversion-artifact download have been updated to mention appversion download-state and vice versa, and the same for the purge commands.

Also, for ease of use and because 'artifacts' are really a stand-alone entity that just happen to be stored at application version level (contrary to for example appversion-user and appversion-attribute; these are part of appversion configuration), I've renamed fcli ssc appversion-artifact to fcli ssc artifact, and implemented a similar change for appversion-vuln (renamed to vulnerability with alias issue)

These changes are available in https://github.com/fortify/fcli/commit/00768485e32d711d10db8730e7923312267f6543