Skip to content

fix: enforce FoD app/release exclusivity and correct session login options#943

Open
SangameshV wants to merge 1 commit intodev/v3.xfrom
fix/v3.x/fod-exclusive-options
Open

fix: enforce FoD app/release exclusivity and correct session login options#943
SangameshV wants to merge 1 commit intodev/v3.xfrom
fix/v3.x/fod-exclusive-options

Conversation

@SangameshV
Copy link
Contributor

Currently, the --app and --release options are not defined as mutually exclusive in PicoCLI. Instead, they are manually validated, and an error is thrown when both or neither of these options are provided for the fcli fod issue list and fcli fod oss-scan list-components commands.

This has now been improved by implementing the PicoCLI approach to define these options as mutually exclusive. As a result, fcli util all-commands list -o json will also correctly reflect these exclusive options.

@SangameshV SangameshV self-assigned this Mar 6, 2026
@SangameshV SangameshV requested a review from rsenden March 6, 2026 12:37
Copy link
Contributor

@rsenden rsenden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now duplicating TargetSpecifierArgGroup, AppTarget, ReleaseTarget across three different classes; please move this to an ArgGroup implementation that can be re-used across all three classes, with proper naming like FoDAppOrReleaseArgGroup.

Also, I noticed that you're extending from AbstractFoD*Mixin, but please note that ArgGroups and Mixins are different things, and in general shouldn't be mixed. For example, there might be generic (injection) functionality that operates on Mixins but not ArgGroups, which I guess you already noticed, given the releaseGroup.setDelimiterMixin() call.

We don't want each command class having to explicitly set the delimiter mixin, so please research alternative options, for example extending the injection functionality to ArgGroups, or by having an FoDAppOrReleaseMixin (which itself is not an ArgGroup, but it may contain the exclusive ArgGroup), with this mixin handling the delimiter mixin injection and selection between app vs release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants