feat: Aviator apply-remediations issue-id filtering for SSC/FoD#1050
feat: Aviator apply-remediations issue-id filtering for SSC/FoD#1050kireetivar wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
I've done a partial review and added some individual code comments, which mostly originate from current (command) design. Can we improve the design, thereby automatically resolving most of these comments?
I think the key issue is that these commands operate on individual FPR files, thus requiring separate --file/--dir output options depending on how many FPRs are being downloaded, and deviating from fcli arity conventions to pass these FPRs through wildcards to apply-remediations commands. Also, when depending on user-specified dirs/wildcards, there's a risk that incorrect FPR files get picked up, for example from a previous run, possibly even from a different app version/release.
Three alternative approaches come to mind (but maybe you can think of other approaches):
- Get rid of
download-remediations-fprcommands, and instead add caching options onapply-remediationscommands, for example a--[no-]use-cacheoption that checks fcli state directory whether cached FPR files for current set of args (--av/--rel& artifact selection criteria) is available, downloading these to the cache on the fly if not yet available or outdated. Main complexity is cache management, for example requiring extra commands/options for updating or clearing cache, detecting outdated cache, ... - Have
download-remediations-fpr(possibly renamed to something likeprepare-remediations-cacheoutput a single 'remediations cache' zip file; depending on FPR selection criteria, this zip-file can contain either single or multiple FPR files, which users can then pass toapply-remediationscmd using--from-cache=<zip-file> - Combination of the above; have
apply-remediations --use-cache=<zip-file>; if given zip-file exists, it's used as is, otherwise it's created by downloading FPR files based on current FPR selection criteria. Possibly, the zip-file could contain metadata like app version/release and FPR selection criteria; if there's a mismatch with current selection criteria on theapply-remediationscommand, an error would be thrown.
|
|
||
| @Override | ||
| public boolean isSingular() { | ||
| return true; |
There was a problem hiding this comment.
What's the rationale for having singular output with artifact id's embedded in that single output record, rather than outputting separate records for every individual artifact? Unless there are good reasons against the latter, that's the more common fcli convention.
| result.putArray("artifactIds").add(artifact.getId()); | ||
| result.putArray("files").add(destination.toString()); | ||
| result.put("file", destination.toString()); | ||
| result.put(IActionCommandResultSupplier.actionFieldName, getActionCommandResult()); |
There was a problem hiding this comment.
fcli output framework automatically sets the 'action' column based on getActionCommandResult; explicitly setting this field is only necessary if you want to display dynamic action values, i.e., if that value depends on execution flow/result.
| files.add(destination.toString()); | ||
| artifactIds.add(artifact.getId()); | ||
| } | ||
| result.put(IActionCommandResultSupplier.actionFieldName, getActionCommandResult()); |
There was a problem hiding this comment.
fcli output framework automatically sets the 'action' column based on getActionCommandResult; explicitly setting this field is only necessary if you want to display dynamic action values, i.e., if that value depends on execution flow/result.
| throw new FcliSimpleException("--dir must be specified when using --all"); | ||
| } | ||
| } else if (outputDir != null) { | ||
| throw new FcliSimpleException("--dir can only be used with --all; use -f/--file for a single FPR download"); |
There was a problem hiding this comment.
Why is --dir not supported for single-file downloads, using the default FPR file name as defined in getSingleDestination.
| private boolean latest; | ||
|
|
||
| @Option(names = {"--all"}, required = true, descriptionKey = "fcli.aviator.ssc.download-remediations-fpr.all") | ||
| private boolean allOpenIssues; |
There was a problem hiding this comment.
Weird field name for 'all FPRs'
|
|
||
| @Option(names = {"--fpr"}, required = true, arity = "1..*", paramLabel = "<file>", | ||
| descriptionKey = "fcli.aviator.ssc.apply-remediations.fpr") | ||
| @DisableTest({TestType.MULTI_OPT_SPLIT, TestType.MULTI_OPT_PLURAL_NAME, TestType.OPT_ARITY_VARIABLE}) |
There was a problem hiding this comment.
I guess the rationale for non-standard arity and the disabled tests is to allow this to work with wildcard patterns, i.e., --fpr /path/to/downloads/*.fpr? Although picocli does its best to handle multi-arity options, we encountered issues with this in the past, hence we usually disallow this.
| return outputFile == null ? Path.of(String.format("remediations_artifact_%s.fpr", artifact.getId())) : outputFile.toPath(); | ||
| } | ||
|
|
||
| private void validateDestinationOptions() { |
There was a problem hiding this comment.
In general, I don't like exclusive sets of options as it affects user experience (difficult to figure out which sets of options can be combined), nor manual validation instead of picocli handling option exclusivity (also to properly display this in command syntax). Any way around this, by reorganizing ArgGroups?
|
Thanks for the review comments. I’m planning to revise the PR away from the current raw-FPR workflow. Proposed solution:
Example {
"schemaVersion": 1,
"kind": "aviator-remediations-cache",
"product": "ssc",
"createdAt": "2026-07-16T12:00:00Z",
"selection": {
"mode": "all",
"appVersionId": "10001",
"since": "2026-07-01T00:00:00Z"
},
"entries": [
{
"order": 1,
"artifactId": "12345",
"uploadDate": "2026-07-10T08:00:00Z",
"path": "fprs/001_artifact_12345.fpr",
"sha256": "8f14e45fceea167a5a36dedd4bea2543"
},
{
"order": 2,
"artifactId": "67890",
"uploadDate": "2026-07-11T09:30:00Z",
"path": "fprs/002_artifact_67890.fpr",
"sha256": "45c48cce2e2d7fbdea1afc51c7c6ad26"
}
]
}
If you are ok with this proposal, I have a few questions I’d like your input on:
|
|
I don't have a strong preference for either option 2 or 3:
For option 2, I think it's better to have For option 3, we would need to verify options against cache, but then need to decide what to do if there's a mismatch; throw an error, replace the cache based on current selection options, ...
Yes, if we go for option 2
I think requiring
Use
Not sure; probably should have same behavior as current non-cache based |
This PR adds remediations cache download/apply workflows for SSC and FoD so teams can download remediations once into a cache zip and apply them locally, including optional issue-level filtering.
What changed
fcli aviator ssc download-remediations-cachefcli fod aviator download-remediations-cachefcli aviator ssc apply-remediationsfcli fod aviator apply-remediations--from-cachesupport to SSC and FoD apply-remediations.--issue-idssupport for cache-based apply-remediations.--artifact-id--latest--all--release/--relremediations.xml.Behavior
--from-cache:--issue-ids:instanceIdmatches requested ids are considered.--issue-idsis only supported together with--from-cache.--issue-ids:remediations.xml:SSC cache behavior
download-remediations-cache --allstores artifacts in chronological order.apply-remediations --from-cacheprocesses entries in the same stored order.FoD cache behavior
download-remediations-cachestores the selected release FPR in the cache zip.apply-remediations --from-cacheprocesses entries in cache order.apply-remediations command additions
fcli aviator ssc apply-remediations --from-cache <zip>fcli aviator ssc apply-remediations --from-cache <zip> --issue-ids <id1,id2,...>fcli fod aviator apply-remediations --from-cache <zip>fcli fod aviator apply-remediations --from-cache <zip> --issue-ids <id1,id2,...>