feat(aviator): Improve apply-remediations UX with --latest, --all-ope…#948
feat(aviator): Improve apply-remediations UX with --latest, --all-ope…#948mneeta wants to merge 2 commits intofortify:feat/v3.x/aviator/26.2from
Conversation
…n-issues, and --since options This enhancement provides a more flexible and user-friendly experience for applying Aviator auto-remediations by introducing multiple artifact selection modes: - --latest: Automatically select the most recent Aviator-processed artifact - --all-open-issues: Process all artifacts with open issues in bulk - --since: Filter artifacts by upload date (relative: 7d, 2w, 1M; absolute: 2025-01-01) Key Changes: - Replaced required --artifact-id with flexible selection modes - Added SinceOptionHelper for robust date/period parsing - Enhanced SSCArtifactHelper with getLatestAviatorArtifact() and getAllAviatorArtifacts() - Improved command validation with mutual exclusivity checks - Added comprehensive unit tests for all new options - Updated i18n messages with detailed usage descriptions Technical Details: - SinceOptionHelper supports relative periods (d, w, M, y) and absolute ISO-8601 dates - DateTimePeriodHelper integration for consistent period parsing across fcli - Proper UTC timezone handling for date comparisons - Backward compatible - existing --artifact-id usage unchanged Closes: #XXX
| AviatorEntitlementCommands.class, | ||
| AviatorSSCCommands.class, | ||
| // AviatorFoDCommands.class, | ||
| AviatorFoDCommands.class, |
There was a problem hiding this comment.
There's some discussion as to whether this should live under fcli aviator or fcli fod. Given that Aviator-related functionality is handled by FoD in the background, none of the other fcli aviator commands is relevant for FoD (like user/admin session management, app & entitlement managent, ...). So, unless we plan on supporting any of this functionality also for FoD, moving this to fcli fod might make more sense.
| } | ||
|
|
||
|
|
||
| private void validateOptions() { |
There was a problem hiding this comment.
Please use Picocli ArgGroups for handling exclusivity and related options, either as inner classes in this class, or as a separate single class (with sub-arggroups as inner classes) in the cli.mixin package.
| AviatorEntitlementCommands.class, | ||
| AviatorSSCCommands.class, | ||
| // AviatorFoDCommands.class, | ||
| AviatorFoDCommands.class, |
There was a problem hiding this comment.
As discussed with Frans, since none of the other fcli aviator commands make sense for FoD (Aviator user/admin session, app & entitlement management), we'll want to move this to the FoD module, i.e., have fcli fod aviator apply-remediations. This likely requires adding a dependency from fcli-fod module on fcli-aviator-common module, and possibly some refactoring (moving code from fcli-aviator to fcli-aviator-common if FoD Aviator commands depend on any code in fcli-aviator module.
| } | ||
|
|
||
|
|
||
| private void validateOptions() { |
There was a problem hiding this comment.
Please use Picocli ArgGroups to model exclusivity and related options, to have Picocli take care of validation and provide consistent error messaging across fcli. The ArgGroups could be either inner classes in this command class, or potentially a single top-level class (with inner classes for sub-arggroups) in the cli.mixin package.
| Path fprPath = Files.createTempFile("aviator_" + ad.getId() + "_", ".fpr"); | ||
| try (IProgressWriter progressWriter = progressWriterFactoryMixin.create()){ | ||
| logger.progress("Status: Downloading Audited FPR from SSC"); | ||
| try (IProgressWriter progressWriter = progressWriterFactoryMixin.create()) { |
There was a problem hiding this comment.
There's already a progress writer instantiated in getJsonNode method; nested progress writers are not supported and could cause issues. Also, to avoid having to pass around unirest, logger, and progressWriter all the time, probably better to have an inner class that takes these through Lombok RequiredArgsConstructor and performs all of the processing.
|
|
||
| JsonNode data = response.get("data"); | ||
| if (data == null || !data.isArray() || data.size() == 0) { | ||
| throw new FcliSimpleException( |
There was a problem hiding this comment.
Should this throw an exception, or simply return null?
| for (JsonNode artifact : data) { | ||
| String originalFileName = artifact.path("originalFileName").asText(""); | ||
| if (!originalFileName.startsWith("aviator_")) { continue; } | ||
| if (sinceDate != null && !isUploadDateOnOrAfter(artifact, sinceDate)) { continue; } |
There was a problem hiding this comment.
Is this logic correct? If upload date for current non-Aviator artifact is older than sinceDate, then all subsequent artifacts will also be older than sinceDate, so once we reach this point, we'll never find an Aviator artifact that's newer than sinceDate.
| return getDescriptor(artifact); | ||
| } | ||
|
|
||
| throw new FcliSimpleException(buildNoArtifactsMessage(appVersionId, sinceDate)); |
There was a problem hiding this comment.
Same as above; null or exception?
|
|
||
| while (true) { | ||
| JsonNode response = unirest.get(SSCUrls.PROJECT_VERSION_ARTIFACTS(appVersionId)) | ||
| .queryString("orderby", "uploadDate ASC") |
There was a problem hiding this comment.
Same comment about orderBy as above, but given that you're trying to order by ascending date, this might have more impact. Also, if a customer has years of uploads, searching from the oldest date might not be very efficient.
| * @return SSCArtifactDescriptor of the most recent qualifying Aviator artifact | ||
| * @throws FcliSimpleException if no matching Aviator artifacts found | ||
| */ | ||
| public static final SSCArtifactDescriptor getLatestAviatorArtifact(UnirestInstance unirest, String appVersionId, OffsetDateTime sinceDate) { |
There was a problem hiding this comment.
Any way to improve this code/have less code duplication between the various Aviator-related methods?
2 participants
This enhancement provides a more flexible and user-friendly experience for applying Aviator auto-remediations for SSC by introducing multiple artifact selection modes:
Enabled apply-remediations command for FoD as well