🐛 Add identity role handling to application manage credentials #2610
Conversation
Partial: https://issues.redhat.com/browse/MTA-6122 Partial: konveyor#2599 Use kind=source identities for asset repositories by adding a role to the `application.identities` array. Removal of the management of kind=asset identities will be done in a later PR. Manage Credentials will retain any identities that do not have a role of source, maven, or asset. Each drop down on the modal assigns a role to the selected identity. Those roles will be used by HUB and AddOns as appropriate instead of relying on the identity kind. Signed-off-by: Scott J Dickerson <[email protected]>
WalkthroughAdds a role-annotated identity type to API models and migrates application identity handling from kind-based refs to role-based Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Form as ApplicationIdentityForm
participant Hook as useFetchIdentities
participant API as Backend API
User->>Form: Open identity form
Form->>Hook: fetch identities
Hook-->>Form: identities, identitiesByKind
Note over Form: Compute defaults using firstIdentityOfRole\n(options derived from identitiesByKind)
User->>Form: Select source/maven/asset
Form->>Form: Validate via hasIdentityOfRole
User->>Form: Submit
Form->>Form: Build updatedIdentities (RefWithRole[])
Form->>Form: Merge with otherIdentitiesPerApplication (preserve non-managed)
Form->>API: PATCH Application.identities
API-->>Form: Success / Error
Form-->>User: Notify result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/app/api/models.ts(2 hunks)client/src/app/pages/applications/application-identity-form/application-identity-form.tsx(7 hunks)client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (4)
client/src/app/api/models.ts (4)
Identity(223-237)RefWithRole(93-95)Ref(85-88)Application(142-167)client/src/app/utils/model-utils.tsx (2)
toRef(238-244)toRefs(256-260)client/src/app/pages/applications/useDecoratedApplications.ts (1)
DecoratedApplication(39-69)client/src/app/queries/identities.ts (1)
useFetchIdentities(58-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (9)
client/src/app/api/models.ts (1)
93-96: Good addition: role-aware ref wrapperOptional role keeps backward compatibility. No issues.
client/src/app/pages/applications/application-identity-form/application-identity-modal.tsx (1)
6-7: Import reorder onlyNo behavioral change. Looks fine.
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (7)
56-65: Helper is correct
identityToRefWithRolebuilds the correct payload shape and tolerates nulls.
66-79: Role-based lookups/readiness checks look good
firstIdentityOfRoleandhasIdentityOfRoleare straightforward and efficient.
87-87: Fetching identities by kindNo issues; memoized
identitiesByKindaligns with usage below.
91-91: Intentional: asset options sourced from kind=sourceMatches PR objective (asset role uses source identities).
If HUB ever exposes dedicated
assetkind again, confirm this still matches product behavior.
135-139: Building managed-role updates is correctType and filtering are appropriate.
199-203: Defaults derived from role: OKGood UX, selecting the first available role assignment.
211-212: Multi-edit warning logic: OKWarns only when batch-editing apps that already have managed roles.
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
Show resolved
Hide resolved
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: konveyor#2599 Follows: konveyor#2610 Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (4)
45-49: Make MANAGED_IDENTITY_ROLES truly readonly (currentas constis redundant)Type the constant as readonly; the current annotation widens it to a mutable array.
-const MANAGED_IDENTITY_ROLES: ManagedIdentityRole[] = [ - "source", - "maven", - "asset", -] as const; +const MANAGED_IDENTITY_ROLES: readonly ManagedIdentityRole[] = [ + "source", + "maven", + "asset", +] as const;
63-71: Helper is correct; consider minor signature tweakIf desired, make
idbenumber | undefinedto avoid passingnull, but current behavior is fine.
73-75: Narrow the role type for better safetyUse the union type for roles.
-function firstIdentityOfRole(application: DecoratedApplication, role: string) { +function firstIdentityOfRole( + application: DecoratedApplication, + role: ManagedIdentityRole +) {
77-86: Type safety and small perf nitType the parameter as
ManagedIdentityRole | ManagedIdentityRole[]and use a Set to avoid repeatedincludes.-export function hasIdentityOfRole( - applications: DecoratedApplication | DecoratedApplication[], - role: string | string[] -) { - const roles = Array.isArray(role) ? role : [role]; +export function hasIdentityOfRole( + applications: DecoratedApplication | DecoratedApplication[], + role: ManagedIdentityRole | ManagedIdentityRole[] +) { + const roles = Array.isArray(role) ? role : [role]; + const roleSet = new Set<ManagedIdentityRole>(roles); const apps = Array.isArray(applications) ? applications : [applications]; return apps.some((app) => - app.identities?.some((i) => i.role && roles.includes(i.role)) + app.identities?.some((i) => i.role && roleSet.has(i.role as ManagedIdentityRole)) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/app/api/models.ts(3 hunks)client/src/app/pages/applications/application-identity-form/application-identity-form.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/app/api/models.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T16:01:04.207Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2583
File: client/src/app/pages/identities/components/identity-form/kind-source-form.tsx:17-26
Timestamp: 2025-08-29T16:01:04.207Z
Learning: Skip i18n/internationalization suggestions for the file client/src/app/pages/identities/components/identity-form/kind-source-form.tsx - hardcoded strings like "Username/Password" and "Private Key/Passphrase" should remain as-is.
Applied to files:
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (3)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (3)
2-2: LGTM: role-based model wiring and importsImports align with the role-based identities shift; dead toRefs removed; new hooks/components correctly referenced.
Also applies to: 5-5, 19-25, 26-29, 34-35
101-104: Asset options sourced from “source” identities: OKMatches the PR intent to use kind=source identities for asset repositories.
153-167: Preservation of non-managed/unknown roles: good recovery from earlier issueThe filter now keeps role-less and unknown roles and avoids
toRefs, so roles are preserved on patch. Nice.
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
Show resolved
Hide resolved
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: konveyor#2599 Follows: konveyor#2610 Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: konveyor#2599 Follows: konveyor#2610 Signed-off-by: Scott J Dickerson <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (1)
147-152: Uniform bulk-apply of managed rolesBuilding the three role-tagged refs is correct for the intended multi-edit behavior (apply form state across all apps). Acknowledging prior discussion/learning that this overwrite is intentional.
🧹 Nitpick comments (4)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (4)
45-49: Type the managed-role set more narrowlyUse a readonly tuple plus a ReadonlySet typed to ManagedIdentityRole for stronger checks and reuse elsewhere.
Apply:
-const MANAGED_IDENTITY_SET: Set<IdentityRole> = new Set([ - "source", - "maven", - "asset", -]); +const MANAGED_IDENTITY_ROLES = ["source", "maven", "asset"] as const; +const MANAGED_IDENTITY_SET: ReadonlySet<ManagedIdentityRole> = new Set( + MANAGED_IDENTITY_ROLES +);
63-71: Helper correctly preserves role on refsLooks good and avoids stripping the role. Micro: if this is called often, consider prebuilding a Map<number, Identity> once, but not required here.
80-87: Minor simplification in managed-role presence checkYou can drop the extra truthy check; Set.has(undefined) is false, preserving behavior.
Apply:
- return apps.some((app) => - app.identities?.some((i) => i.role && MANAGED_IDENTITY_SET.has(i.role)) - ); + return apps.some((app) => + app.identities?.some((i) => MANAGED_IDENTITY_SET.has(i.role as ManagedIdentityRole)) + );
319-326: Consider enabling Save in multi-edit even when not dirtyRight now, Save is disabled until a change is made. For bulk standardization, users may want to open the modal and immediately apply the first app’s defaults to the rest. Allow submit when applications.length > 1 even if the form is pristine.
Apply:
- isDisabled={!isValid || isSubmitting || isValidating || !isDirty} + isDisabled={ + !isValid || + isSubmitting || + isValidating || + (!isDirty && applications.length === 1) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T16:01:04.207Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2583
File: client/src/app/pages/identities/components/identity-form/kind-source-form.tsx:17-26
Timestamp: 2025-08-29T16:01:04.207Z
Learning: Skip i18n/internationalization suggestions for the file client/src/app/pages/identities/components/identity-form/kind-source-form.tsx - hardcoded strings like "Username/Password" and "Private Key/Passphrase" should remain as-is.
Applied to files:
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
📚 Learning: 2025-09-12T18:54:17.513Z
Learnt from: sjd78
PR: konveyor/tackle2-ui#2610
File: client/src/app/pages/applications/application-identity-form/application-identity-form.tsx:146-151
Timestamp: 2025-09-12T18:54:17.513Z
Learning: In ApplicationIdentityForm multi-edit mode, the form state is intentionally applied as-is across all selected applications. This means that even untouched form fields will overwrite existing values across all applications, which is the expected bulk-edit behavior for standardizing identity credentials across multiple applications.
Applied to files:
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx
🧬 Code graph analysis (1)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (5)
client/src/app/api/models.ts (5)
IdentityRole(143-143)Identity(226-240)ManagedIdentityRole(142-142)RefWithRole(93-95)Application(145-170)client/src/app/utils/model-utils.tsx (1)
toRef(238-244)client/src/app/pages/applications/useDecoratedApplications.ts (1)
DecoratedApplication(39-69)client/src/app/components/NotificationsContext.tsx (1)
NotificationsContext(31-33)client/src/app/queries/identities.ts (1)
useFetchIdentities(58-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (7)
client/src/app/pages/applications/application-identity-form/application-identity-form.tsx (7)
19-25: Imports updated correctly for role-aware identitiesGood switch to RefWithRole/IdentityRole and removal of toRefs; HookFormPF, NotificationsContext, and SimpleSelect imports look right.
Also applies to: 26-28, 34-34
73-78: Role-based lookup aligns with the new modelSimple and correct replacement for kind-based defaults.
100-105: Asset options sourced from source-kind identitiesThis matches the PR objective to use kind=source identities for asset repositories. Good catch on switching only the asset list.
155-166: Correct retention of non-managed or role-less identitiesThe filter now preserves entries with unknown/undefined roles and keeps their role field intact (no toRefs). This aligns with “retain any identities that do not have a role of source, maven, or asset.”
168-175: Patch construction is safe and order-stable enoughMerges updated managed roles with preserved others per application. Looks good.
213-216: Defaults from first app are fine for bulk-editConsistent with the intended multi-edit semantics: form initializes from applications[0] and applies uniformly.
221-225: Good UX: warning for multi-edit overridesThe check properly warns when editing multiple apps and any has managed identities already.
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: konveyor#2599 Follows: konveyor#2610 Signed-off-by: Scott J Dickerson <[email protected]>
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: #2599 Remove display and CRUD support for `kind=asset` identities from the Credentials pages. #2610 adds the use of a `role` on the `application.identities` Ref[] to indicate how each reference identity should be used. Signed-off-by: Scott J Dickerson <[email protected]>
…yor#2610) Part 1 of 2: https://issues.redhat.com/browse/MTA-6122 Part 1 of 2: konveyor#2599 Requires hub changes in: konveyor/tackle2-hub#911 Use kind=source identities for asset repositories by adding a role to the `application.identities` array. Removal of the management of kind=asset identities will be done in a later PR. The identity `Ref` roles will be used by HUB and AddOns as appropriate instead of relying on the identity's kind. Manage Credentials modal: - Remains unchanged visually - Will retain any identities that do not have a role of source, maven, or asset - Each drop down assigns a role to the selected identity ## Summary by CodeRabbit * **New Features** * Role-based identity management for applications with role-aware defaults and selection (source, maven, asset). * **Bug Fixes** * Preserves existing non-managed identities when updating. * Improved validation and clearer error feedback to prevent duplicate or mismatched identities. * **Refactor** * Consolidated identities into a unified top-level structure and streamlined identity fetching for more consistent form behavior. --------- Signed-off-by: Scott J Dickerson <[email protected]>
Part 2 of 2: https://issues.redhat.com/browse/MTA-6122 Part 2 of 2: konveyor#2599 Remove display and CRUD support for `kind=asset` identities from the Credentials pages. konveyor#2610 adds the use of a `role` on the `application.identities` Ref[] to indicate how each reference identity should be used. Signed-off-by: Scott J Dickerson <[email protected]>
2 participants
Part 1 of 2: https://issues.redhat.com/browse/MTA-6122
Part 1 of 2: #2599
Requires hub changes in: konveyor/tackle2-hub#911
Use kind=source identities for asset repositories by adding a role to the
application.identitiesarray. Removal of the management of kind=asset identities will be done in a later PR. The identityRefroles will be used by HUB and AddOns as appropriate instead of relying on the identity's kind.Manage Credentials modal:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor