Support organization-based CF discovery and filtering #587
Conversation
WalkthroughRefactors Cloud Foundry discovery to be organization-centric: adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant DiscoverSvc
participant CF
rect rgb(245,250,255)
Note over User,CLI: Org-centric discovery
User->>CLI: discover --orgs org1 [--spaces ...] [--list-apps]
CLI->>DiscoverSvc: StartDiscover(orgs=[org1], spaces=opt)
end
loop per org
DiscoverSvc->>CF: ListSpaces(org1)
CF-->>DiscoverSvc: spaces[]
alt spaces found
loop per space
DiscoverSvc->>CF: ListApps(space)
CF-->>DiscoverSvc: apps[]
DiscoverSvc->>CLI: accumulate {org1:{space:[apps...]}}
end
else no spaces
DiscoverSvc-->>CLI: log "no matching spaces/apps for org1"
end
end
DiscoverSvc-->>CLI: result {org:{space:[apps...]}}
CLI->>CLI: print Organization → Space → App (includes orgName)
CLI->>CLI: write manifests with orgName in filenames
CLI-->>User: output / files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
gciavarrini
force-pushed
the
MTA-6271-add-org-id
branch
from
9ed40a8 to
0f3cca1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #587 +/- ##
==========================================
+ Coverage 28.68% 30.14% +1.46%
==========================================
Files 32 32
Lines 3828 3868 +40
==========================================
+ Hits 1098 1166 +68
+ Misses 2592 2569 -23
+ Partials 138 133 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/asset_generation/discover/cloud_foundry/cmd.go(8 hunks)cmd/asset_generation/discover/cloud_foundry/cmd_test.go(7 hunks)docs/examples.md(1 hunks)go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/asset_generation/discover/cloud_foundry/cmd_test.gocmd/asset_generation/discover/cloud_foundry/cmd.go
🧬 Code graph analysis (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (1)
NewDiscoverCloudFoundryCommand(39-104)
🪛 LanguageTool
docs/examples.md
[grammar] ~82-~82: Ensure spelling is correct
Context: ...1,space2 --list-apps` - Perform a live discover and print the YAML representation of so...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~88-~88: Ensure spelling is correct
Context: ...spaces=space1,space2` - Perform a live discover and output the YAML representations of ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~95-~95: Ensure spelling is correct
Context: ...-dir=/tmp/output-dir` - Perform a live discover of a specific application and output th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
gciavarrini
force-pushed
the
MTA-6271-add-org-id
branch
from
0f3cca1 to
b1a6a3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (1)
133-149: PropagateprintAppserrors to the caller.
printAppsnow returns an error (line 171), but both call sites ignore it:
- Line 146 in
listApplicationsLive- Line 166 in
listApplicationsLocalThis means failures in
printApps(e.g., unexpected app types) will be silently swallowed, leaving users with incomplete output and no indication of what went wrong.Apply this diff to handle errors in
listApplicationsLive:- printApps(appListPerSpace, out) - - return nil + return printApps(appListPerSpace, out)Apply the same fix to
listApplicationsLocalat line 166:} - printApps(appListPerSpace, out) + if err := printApps(appListPerSpace, out); err != nil { + return err + } }Also applies to: 150-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/asset_generation/discover/cloud_foundry/cmd.go(8 hunks)cmd/asset_generation/discover/cloud_foundry/cmd_test.go(10 hunks)docs/examples.md(2 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/asset_generation/discover/cloud_foundry/cmd_test.go
🧬 Code graph analysis (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (2)
NewDiscoverCloudFoundryCommand(39-110)OutputAppManifestsYAML(401-467)
🪛 LanguageTool
docs/examples.md
[grammar] ~76-~76: Ensure spelling is correct
Context: ...-dir=/tmp/output-dir` - Perform a live discover and print the list of the available app...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~86-~86: Ensure spelling is correct
Context: ...1,space2 --list-apps` - Perform a live discover and print the YAML representation of so...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~95-~95: Ensure spelling is correct
Context: ...spaces=space1,space2` - Perform a live discover and output the YAML representations of ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~105-~105: Ensure spelling is correct
Context: ...-dir=/tmp/output-dir` - Perform a live discover of a specific application and output th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (5)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
73-110: Excellent test coverage for org-centric discovery model.The test suite comprehensively covers the new organization-centric discovery model:
- Hierarchical output validation (org→space→apps)
- Optional flag behavior (
--orgsand--spaces)- Error handling for invalid types and failed operations
- Edge cases like empty organizations, unknown spaces, and filtering
- Updated function signatures with
orgNameparameterThe tests follow proper Ginkgo/Gomega patterns and provide good coverage of both success and failure paths.
Also applies to: 416-834, 1014-1114
cmd/asset_generation/discover/cloud_foundry/cmd.go (4)
50-65: Well-designed validation for org-centric discovery.The validation logic correctly enforces the new requirements:
--orgsis only allowed with--use-live-connection--orgsis required for discovery mode (but optional for--list-apps)- Clear, actionable error messages guide users
This aligns well with the PR objective of making organizations central to discovery while maintaining backward compatibility for listing operations.
171-212: Well-structured hierarchical output implementation.The refactored
printAppsfunction correctly implements the organization-centric model:
- Builds a clear
org → space → appshierarchy- Handles edge cases (empty space names default to "unknown")
- Returns errors for unexpected types instead of panicking
- Produces readable hierarchical output with proper indentation
The implementation is clean and maintainable.
270-280: Helpful auto-discovery logging improves user experience.The informational logs clearly communicate when filters are omitted and what the discovery scope will be. This helps users understand what's happening when they don't specify
--orgsor--spaces, making the auto-discovery behavior transparent and predictable.
295-340: Org-centric discovery flow correctly implemented.The
discoverLivefunction properly implements the organization-first model:
- Iterates through organizations, then their applications
- Logs organizations that are skipped due to filters or no apps
- Passes complete context (
orgName,spaceName,appName) through the pipelineThe updated
OutputAppManifestsYAMLsignature correctly accepts and usesorgNamefor file naming, ensuring manifests are uniquely identified by their full organizational hierarchy.Also applies to: 401-408
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/asset_generation/discover/cloud_foundry/cmd.go (2)
205-214: Consider sorting for deterministic output order.The nested map iteration produces non-deterministic output order because Go maps don't preserve insertion order. This could make it harder for users to find specific apps or compare outputs across runs.
Consider sorting organizations, spaces, and apps alphabetically before printing:
// Print in org -> space -> apps hierarchy +// Sort orgs for deterministic output +orgNames := make([]string, 0, len(hierarchy)) +for orgName := range hierarchy { + orgNames = append(orgNames, orgName) +} +sort.Strings(orgNames) + -for orgName, spaces := range hierarchy { +for _, orgName := range orgNames { + spaces := hierarchy[orgName] fmt.Fprintf(out, "Organization: %s\n", orgName) + + // Sort spaces for deterministic output + spaceNames := make([]string, 0, len(spaces)) + for spaceName := range spaces { + spaceNames = append(spaceNames, spaceName) + } + sort.Strings(spaceNames) + - for spaceName, apps := range spaces { + for _, spaceName := range spaceNames { + apps := spaces[spaceName] fmt.Fprintf(out, " Space: %s\n", spaceName) + + // Sort apps for deterministic output + sort.Strings(apps) for _, appName := range apps { fmt.Fprintf(out, " - %s\n", appName) } } }You'll need to import
sortif not already imported.
405-412: Consider defensive validation for empty appName.While
appNameshould always be provided by the caller with a valid value fromappRef.AppName, adding a validation check would make the code more robust against unexpected provider data.Consider adding a validation check at the start of the function:
func OutputAppManifestsYAML(out io.Writer, discoverResult *providerTypes.DiscoverResult, orgName string, spaceName string, appName string) error { + if appName == "" { + return fmt.Errorf("appName cannot be empty") + } + suffix := "_" + appName if spaceName != "" { suffix = "_" + spaceName + suffixThis would catch any data quality issues from the provider early and provide a clear error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go(8 hunks)
🔇 Additional comments (5)
cmd/asset_generation/discover/cloud_foundry/cmd.go (5)
50-65: LGTM! Validation logic is well-structured.The validation correctly enforces that:
--orgsis only used with live connections--orgsis required for live discovery mode (but optional for--list-apps)The error messages clearly explain the requirements to users.
146-148: Past review comment addressed: error now properly propagated.The error returned by
printAppsis now correctly captured and propagated to the caller, preventing silent failures when app listing encounters issues.
168-170: Past review comment addressed: error handling consistent across both paths.Both
listApplicationsLiveandlistApplicationsLocalnow properly propagateprintAppserrors, ensuring consistent error handling.
274-284: LGTM! Helpful auto-discovery logging.The logging provides clear visibility into what will be discovered when users don't specify organizations or spaces, improving the user experience and making debugging easier.
299-344: LGTM! org-centric discovery correctly implemented.The refactored
discoverLivefunction properly:
- Iterates through organizations rather than spaces
- Logs organizations with no matching spaces/apps
- Propagates
orgNamethrough the discovery workflow- Provides comprehensive logging for each org and app being processed
gciavarrini
force-pushed
the
MTA-6271-add-org-id
branch
from
a8ba3aa to
8be2287
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/examples.md (2)
62-62: Fix duplicated path segments in examples.The paths contain a duplicated
discover/discover/segment that should be corrected todiscover/to match other examples in the file (line 55).Apply this diff to fix the paths:
- --input=./test-data/asset_generation/discover/discover/cf-sample-app.yaml --list-apps` + --input=./test-data/asset_generation/discover/cf-sample-app.yaml --list-apps`Make the same correction on lines 68 and 74.
Also applies to: 68-68, 74-74
76-76: Correct grammar: use "live discovery" instead of "live discover".The phrase "Perform a live discover" is grammatically incorrect. It should be "Perform live discovery" or "Perform a live discovery."
Apply this diff to correct the grammar on all four occurrences:
-- Perform a live discover and print the list of the available applications for +- Perform live discovery and print the list of the available applications forMake the same correction on lines 86, 95, and 105.
Also applies to: 86-86, 95-95, 105-105
🧹 Nitpick comments (2)
cmd/asset_generation/discover/cloud_foundry/cmd.go (2)
26-26: Consider refactoring flag variables to local scope.The new
orgsvariable follows the existing pattern of package-level flag variables in this file. However, based on learnings, package-level variables for flags should be avoided to prevent shared mutable state across commands and tests. All flag variables (includingspaces,appName,useLive, etc.) should ideally be defined as local variables withinNewDiscoverCloudFoundryCommand.This refactor would be a broader change affecting the entire file, so it could be deferred to a separate PR focused on improving command initialization patterns.
Based on learnings
139-144: Improve variable naming for clarity.The logging logic is good for observability, but the variable name
appListPerSpaceat line 134 is misleading—it's now a map keyed by organization name, not space. Consider renaming toappListByOrgorappListPerOrgto match the org-centric model and improve code clarity.Apply this diff to improve naming consistency:
- appListPerSpace, err := p.ListApps() + appListByOrg, err := p.ListApps() if err != nil { return fmt.Errorf("failed to list apps by space: %w", err) } // Log orgs that have no spaces/apps and will be skipped for _, org := range orgs { - if appList, exists := appListPerSpace[org]; !exists || len(appList) == 0 { + if appList, exists := appListByOrg[org]; !exists || len(appList) == 0 { logger.Info("Skipping organization: no spaces matching the filter or no applications found", "org_name", org) } } - if err := printApps(appListPerSpace, out); err != nil { + if err := printApps(appListByOrg, out); err != nil { return err }Also update the error message at line 136 from "failed to list apps by space" to "failed to list apps by org" for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/asset_generation/discover/cloud_foundry/cmd.go(8 hunks)cmd/asset_generation/discover/cloud_foundry/cmd_test.go(10 hunks)docs/examples.md(2 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/asset_generation/discover/cloud_foundry/cmd_test.go
🧬 Code graph analysis (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (2)
NewDiscoverCloudFoundryCommand(39-110)OutputAppManifestsYAML(405-471)
🪛 LanguageTool
docs/examples.md
[grammar] ~76-~76: Ensure spelling is correct
Context: ...-dir=/tmp/output-dir` - Perform a live discover and print the list of the available app...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~86-~86: Ensure spelling is correct
Context: ...1,space2 --list-apps` - Perform a live discover and print the YAML representation of so...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~95-~95: Ensure spelling is correct
Context: ...spaces=space1,space2` - Perform a live discover and output the YAML representations of ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~105-~105: Ensure spelling is correct
Context: ...-dir=/tmp/output-dir` - Perform a live discover of a specific application and output th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (28)
cmd/asset_generation/discover/cloud_foundry/cmd.go (10)
50-53: LGTM!The validation correctly prevents
--orgsusage with local manifest discovery and provides a clear error message explaining the default behavior.
62-65: LGTM!The validation correctly enforces that
--orgsis required for discovery mode (non-list-apps) when using live connections, aligning with the org-centric model.
98-99: LGTM!The flag descriptions accurately reflect the new org-centric model:
--spacesis optional and--orgsis required for live discovery.
146-148: LGTM!The error from
printAppsis now correctly propagated, addressing the past review comment. This ensures that failures in printing applications are surfaced to the caller rather than silently ignored.
168-170: LGTM!Error propagation from
printAppsis correctly implemented inlistApplicationsLocal, matching the pattern inlistApplicationsLive.
175-216: LGTM!The refactored
printAppsfunction correctly implements the hierarchical org → space → apps output structure. The error handling for unexpected types, defensive handling of empty space names (defaulting to "unknown"), and clear formatting with proper indentation all look good.
274-284: LGTM!The logging statements provide excellent user feedback about discovery scope, helping users understand what will be discovered based on their flag combinations. This improves observability and user experience.
299-344: LGTM!The
discoverLivefunction correctly implements org-centric discovery with proper variable naming (appListByOrg), comprehensive logging, and correct iteration through organizations. The updated call toOutputAppManifestsYAMLwithorgNamealigns with the new signature.
405-412: LGTM!The updated
OutputAppManifestsYAMLsignature correctly includesorgName, and the suffix building logic properly constructs hierarchical filenames in the format_orgName_spaceName_appName. The empty org name handling is appropriate.
397-397: LGTM!The
OutputAppManifestsYAMLcall correctly passesappRef.OrgNameto support org-aware manifest output naming.cmd/asset_generation/discover/cloud_foundry/cmd_test.go (12)
73-110: LGTM!The updated test table correctly includes the
orgsArgparameter and properly constructs command-line arguments with--orgs. Both test entries appropriately include organization context.
143-176: LGTM!The flag validation tests correctly verify the new org-centric requirements, including the mandatory
--orgsflag for discovery mode with live connections. The error message validation is specific and appropriate.
416-454: LGTM!The test correctly validates the new hierarchical output format (Organization → Space → Apps) with proper indentation and structure. The mock data appropriately represents the org-keyed model.
455-480: LGTM!The test appropriately validates the behavior when organizations have no applications, ensuring organizations are listed but no space or app entries appear.
514-532: LGTM!The test correctly validates error handling in
printAppswhen receiving invalid app types, ensuring errors are properly propagated with meaningful messages.
535-563: LGTM!The tests correctly validate optional flag behavior:
--list-appsdoesn't require--orgs, and--spacesis optional for discovery. The expectations appropriately distinguish between validation passing and execution failing due to environment issues.
603-633: LGTM!The
printAppsfunction tests provide good coverage of edge cases (empty space names) and error conditions (invalid types), using the correct org-keyed data structure.
636-678: LGTM!The
runListAppstests effectively validate routing logic between live and local discovery modes, including error propagation. The use of a flag variable to verify the correct function is called is a good testing practice.
680-718: LGTM!The
listApplicationsLocaltests provide comprehensive error case coverage, including filesystem issues and manifest validation failures. Error expectations are appropriately specific.
720-868: LGTM!The
discoverLivetests provide excellent coverage of success paths, error handling, and edge cases. TheBeforeEachhook properly resets global variables to ensure test isolation, which is important given the package-level flag variables.
1048-1069: LGTM!The
OutputAppManifestsYAMLtests cover different combinations of org/space/app parameters, validating that the function handles optional organization and space names correctly.
1071-1148: LGTM!The
processAppListtests provide thorough coverage of app processing logic, including filtering, error handling, and type validation. The test isolation throughBeforeEachand result tracking throughprocessedAppsare good practices.docs/examples.md (6)
50-50: LGTM!The documentation correctly states that the organization name defaults to 'local' for local manifest discovery, aligning with the implementation.
77-84: LGTM!The documentation correctly explains that
--orgsand--spacesare optional for list-apps mode, with clear examples showing both unfiltered and filtered usage.
86-93: LGTM!The examples correctly demonstrate live discovery with mandatory
--orgsand optional--spacesfiltering, aligning with the new org-centric model.
96-103: LGTM!The documentation accurately shows output directory usage with org/space filtering options, consistent with the implemented behavior.
105-112: LGTM!The app-specific discovery examples correctly demonstrate the use of
--app-namewith org-based filtering, showing both with and without space filtering.
114-121: LGTM!The sensitive data handling examples properly document the
--conceal-sensitive-dataflag with org/space filtering options.
gciavarrini
force-pushed
the
MTA-6271-add-org-id
branch
from
8be2287 to
1c4b72e
Compare
Adapt kantra to use the new organization-centric Cloud Foundry discovery model from asset-generation. Discovery now groups apps by org rather than space, making --orgs required for discovery mode while --spaces becomes optional. The --list-apps output now shows a hierarchical view with organizations at the top level, then spaces, then apps. When no orgs are specified with --list-apps, all organizations are auto-discovered. Signed-off-by: Gloria Ciavarrini <[email protected]>
Signed-off-by: Gloria Ciavarrini <[email protected]>
gciavarrini
force-pushed
the
MTA-6271-add-org-id
branch
from
74e02d8 to
4b1a403
Compare
aufi
left a comment
aufi
left a comment
There was a problem hiding this comment.
Thanks for providing example commands, overall LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (1)
163-163: Fix typo: "Analizing" should be "Analyzing".Apply this diff:
- logger.Info("Analizing manifests file", "Manifest", manifestPath) + logger.Info("Analyzing manifests file", "Manifest", manifestPath)
♻️ Duplicate comments (1)
docs/examples.md (1)
62-62: Fix path inconsistency: remove duplicateddiscover/segment.The path contains
discover/discover/which is inconsistent with line 55 that uses the correct path./test-data/asset_generation/discover/cf-sample-app.yaml. This duplication will cause the command to fail as the file won't be found at the incorrect path.This same issue exists on lines 68 and 74. Apply this diff to all three locations:
- --input=./test-data/asset_generation/discover/discover/cf-sample-app.yaml --list-apps` + --input=./test-data/asset_generation/discover/cf-sample-app.yaml --list-apps`Note: This was flagged in a previous review and marked as addressed, but the fix doesn't appear to have been applied correctly.
🧹 Nitpick comments (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (1)
139-144: Consider extracting the skipped-org logging into a helper function.The same logging pattern appears here and in
discoverLive(lines 305-309). Extracting it would reduce duplication and ensure consistency.Example:
func logSkippedOrgs(appListByOrg map[string][]any, orgs []string, logger logr.Logger) { for _, org := range orgs { if appList, exists := appListByOrg[org]; !exists || len(appList) == 0 { logger.Info("Skipping organization: no spaces matching the filter or no applications found", "org_name", org) } } }Then replace both occurrences with:
logSkippedOrgs(appListPerSpace, orgs, logger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/asset_generation/discover/cloud_foundry/cmd.go(8 hunks)cmd/asset_generation/discover/cloud_foundry/cmd_test.go(11 hunks)docs/examples.md(2 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/asset_generation/discover/cloud_foundry/cmd_test.go
- go.mod
🔇 Additional comments (7)
cmd/asset_generation/discover/cloud_foundry/cmd.go (6)
50-65: LGTM! Validation logic correctly enforces the new org-centric model.The validation appropriately:
- Restricts
--orgsto live discovery mode (lines 50-53)- Requires
--orgsfor discovery operations without--list-apps(lines 62-65)- Provides clear, actionable error messages
146-148: LGTM! Error propagation properly implemented.This correctly addresses the previous review comment by propagating errors from
printAppsinstead of silently ignoring them. The same fix is applied inlistApplicationsLocal(lines 168-170).
175-216: LGTM! Hierarchical printing logic correctly implements org-centric display.The refactored
printAppsfunction properly:
- Groups applications by org → space → app hierarchy
- Handles missing space names gracefully (line 197-199)
- Returns errors for unexpected types (line 193)
- Produces the hierarchical output format described in the PR objectives
274-289: LGTM! Provider configuration and logging correctly support the new model.The auto-discovery logging (lines 274-284) appropriately handles both filtered and unfiltered scenarios, and the config setup (lines 288-289) correctly propagates the org and space filters to the provider.
300-344: LGTM! Live discovery correctly refactored for org-centric model.The function now properly:
- Iterates through organizations first (line 313)
- Logs processing at the org level (line 318)
- Filters applications by name when specified (lines 326-330)
- Passes
orgNametoOutputAppManifestsYAML(line 338)
405-412: LGTM! Function signature correctly updated for org-centric output.The addition of the
orgNameparameter and its inclusion in the suffix (lines 410-412) ensure output files are properly organized by organization. All call sites (lines 338, 397) correctly pass the org name.docs/examples.md (1)
76-122: LGTM! Documentation accurately reflects the new org-centric discovery model.The examples correctly distinguish between:
--list-appsmode where both--orgsand--spacesare optional (line 77-84)- Discovery mode where
--orgsis required and--spacesis optional (lines 86-121)The updated command syntax and explanatory text align with the code validation logic and provide clear guidance for users.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/asset_generation/discover/cloud_foundry/cmd_test.go
🧬 Code graph analysis (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (2)
NewDiscoverCloudFoundryCommand(39-110)OutputAppManifestsYAML(405-471)
⏰ 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (6)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (6)
131-152: Good practice for preventing cross-test contamination.The global variable resets in BeforeEach and AfterEach properly isolate test state, which aligns with best practices for avoiding shared mutable state across tests.
Based on learnings
73-110: LGTM!The test table properly updated to accept and validate
orgsargument for live discovery scenarios. Test entries correctly pass both orgs and spaces for validation.
183-194: LGTM!This test correctly validates the new requirement that
--orgsflag is mandatory when using--use-live-connectionin discovery mode (without--list-apps).
573-601: LGTM!These tests correctly validate the new optional flag behavior:
--list-appsworks without--orgs, and discovery mode works without--spaces. The tests appropriately verify flag validation passes while acknowledging that execution fails due to missing CF environment.
652-944: Excellent comprehensive test coverage.These new function tests provide thorough coverage of the organization-centric discovery model:
printApps: handles edge cases like empty space names and invalid typesrunListApps: properly routes between live and local discovery pathslistApplicationsLocal: covers error scenariosdiscoverLive: comprehensive multi-org, filtering, and error handling testsThe mock provider pattern is used effectively throughout.
1124-1230: LGTM!These tests properly validate the updated function signatures for organization-centric discovery. The
OutputAppManifestsYAMLtests verify the neworgNameparameter integration, and theprocessAppListtests provide good coverage of filtering and error scenarios.
fixing the coverage issue. Thank you for the review |
gciavarrini
force-pushed
the
MTA-6271-add-org-id
branch
from
68b0d99 to
25825c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
131-153: Consider consolidating the duplicate reset logic.The global variable reset logic is duplicated between
BeforeEachandAfterEach. SinceBeforeEachruns before each test, theAfterEachcleanup is redundant for preventing cross-test contamination. You could simplify by keeping only theBeforeEachblock, or extract the reset logic into a helper function if both are needed for other reasons.Apply this diff to reduce duplication:
+func resetGlobalTestState() { + outputDir = "" + appName = "" + orgs = []string{} + spaces = []string{} + input = "" + useLive = false + listApps = false + concealSensitiveData = false +} + BeforeEach(func() { log = logr.Discard() out.Reset() err.Reset() tempDir = createTempDir() manifestPath = createTestManifest(tempDir) outputPath = createOutputDir(tempDir) - // Reset global variables to prevent cross-test contamination - outputDir = "" - appName = "" - orgs = []string{} - spaces = []string{} - input = "" - useLive = false - listApps = false - concealSensitiveData = false + resetGlobalTestState() }) AfterEach(func() { os.RemoveAll(tempDir) - // Clean up global variables to prevent cross-test contamination - outputDir = "" - appName = "" - orgs = []string{} - spaces = []string{} - input = "" - useLive = false - listApps = false - concealSensitiveData = false + resetGlobalTestState() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/asset_generation/discover/cloud_foundry/cmd_test.go
🧬 Code graph analysis (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (2)
NewDiscoverCloudFoundryCommand(39-110)OutputAppManifestsYAML(405-471)
⏰ 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (8)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (8)
73-110: LGTM! Test signature updated to support org-centric discovery.The test table now properly accepts and passes
orgsArgandspacesArgparameters, aligning with the new organization-based discovery model.
183-207: LGTM! Comprehensive flag validation for org-centric discovery.The tests properly verify that
--orgsis required for live discovery mode and can only be used with--use-live-connection, ensuring correct CLI behavior.
475-539: LGTM! Comprehensive hierarchical output validation.The tests thoroughly verify the new org→space→apps hierarchy with proper formatting and handle edge cases like empty app lists.
573-636: LGTM! Good coverage of error cases and optional flag scenarios.The tests properly verify error handling when invalid types are passed and confirm that optional flags work as expected without unnecessary requirements.
687-734: LGTM! Thorough edge case coverage for printApps.The tests properly verify handling of empty space names, mixed scenarios, and type validation errors.
737-819: LGTM! Comprehensive coverage of routing logic and error scenarios.The tests properly verify that
runListAppsdispatches to the correct implementation and thatlistApplicationsLocalhandles various error conditions appropriately.
821-979: LGTM! Excellent comprehensive test coverage for discoverLive.The test suite thoroughly covers multi-org discovery, filtering, error handling, and edge cases like empty org names and invalid types.
1187-1265: LGTM! Comprehensive test coverage for processAppList.The tests thoroughly verify app processing, filtering, and error handling with appropriate mock verification.
Signed-off-by: Gloria Ciavarrini <[email protected]>
gciavarrini
force-pushed
the
MTA-6271-add-org-id
branch
from
25825c5 to
7a69fb9
Compare
Signed-off-by: Gloria Ciavarrini <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/examples.md (1)
76-76: Grammar issue: "Perform a live discover" should be "Perform live discovery."This issue was previously flagged but remains on lines 76, 86, 95, and 105. Replace "Perform a live discover" with "Perform live discovery" for grammatical correctness and consistency.
- Perform a live discover and print the list of the available applications for + Perform live discovery and print the list of the available applications forApply the same correction on lines 86, 95, and 105.
Also applies to: 86-86, 95-95, 105-105
🧹 Nitpick comments (1)
docs/examples.md (1)
81-81: Clarify --orgs requirement for live discovery.Line 81 shows a
--list-appsexample without--orgs, while line 84 (same command) includes--orgs=org1. The comment on line 77 suggests--orgsis optional, but subsequent examples (lines 90, 100, 109, 118) show--orgsas present in all live discovery manifests output examples.Consider explicitly documenting whether
--orgsis:
- Always optional for all live discovery operations
- Required only for manifest output (not for
--list-apps)- Required in all cases
This will prevent user confusion about when the flag is actually needed.
Also applies to: 84-84
The --orgs flag is now required for all live discovery operations to prevent unexpected behavior. Improved coverage and fixed docs flag requirements. Signed-off-by: Gloria Ciavarrini <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/asset_generation/discover/cloud_foundry/cmd.go(8 hunks)cmd/asset_generation/discover/cloud_foundry/cmd_test.go(13 hunks)docs/examples.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/asset_generation/discover/cloud_foundry/cmd_test.go
🧬 Code graph analysis (1)
cmd/asset_generation/discover/cloud_foundry/cmd_test.go (1)
cmd/asset_generation/discover/cloud_foundry/cmd.go (2)
NewDiscoverCloudFoundryCommand(39-110)OutputAppManifestsYAML(405-471)
🪛 LanguageTool
docs/examples.md
[grammar] ~76-~76: Ensure spelling is correct
Context: ...-dir=/tmp/output-dir` - Perform a live discover and print the list of the available app...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~86-~86: Ensure spelling is correct
Context: ...1,space2 --list-apps` - Perform a live discover and print the YAML representation of so...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~95-~95: Ensure spelling is correct
Context: ...spaces=space1,space2` - Perform a live discover and output the YAML representations of ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~105-~105: Ensure spelling is correct
Context: ...-dir=/tmp/output-dir` - Perform a live discover of a specific application and output th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (7)
cmd/asset_generation/discover/cloud_foundry/cmd.go (4)
50-65: LGTM! Clear validation logic with helpful error messages.The validation ensures
--orgsis only used with live connections and is required for all live discovery operations. The error messages clearly explain the constraints to users.
175-216: LGTM! Well-structured hierarchical output.The refactored
printAppsfunction correctly builds and displays the org→space→app hierarchy. The handling of empty space names (defaulting to "unknown") prevents output issues, and proper type checking with error returns ensures robustness.
299-345: LGTM! Org-centric discovery flow with good logging.The refactored
discoverLivefunction correctly iterates by organization and provides useful logging at each level (org processing, app filtering, app discovery). The integration with the updatedOutputAppManifestsYAMLsignature properly includes org context.
405-412: LGTM! Signature update correctly includes org context.The
OutputAppManifestsYAMLfunction signature now includesorgName, and the suffix-building logic properly incorporates it. All call sites (lines 338, 397) have been updated consistently.cmd/asset_generation/discover/cloud_foundry/cmd_test.go (3)
124-153: LGTM! Proper global state management.The BeforeEach and AfterEach hooks correctly reset all global variables between tests, preventing cross-test contamination. This is especially important given the package-level flag variables pattern used in this codebase.
183-207: LGTM! Comprehensive validation testing.The tests properly verify both validation rules:
--orgsis required when using--use-live-connection--orgscannot be used without--use-live-connectionThe error message assertions ensure users get clear feedback.
447-986: LGTM! Excellent test coverage for org-based discovery.The tests comprehensively cover:
- Hierarchical org→space→app output formatting
- Empty organization handling
- Error propagation from printApps
- Multi-org discovery scenarios
- App filtering by name
- Edge cases (empty orgs, invalid types, discovery failures)
The mock providers are used effectively, and assertions verify both success and error paths.
|
@aufi can you please re-review? I had to fix a bug regarding |
Resolved conflict in cmd/command-context_test.go by accepting main's version which uses the newer logr.Discard() pattern instead of logrus. This brings the test suite up to date with recent changes: - Hybrid analysis mode support (konveyor#592) - CF discovery filtering (konveyor#587) - Stale issue workflow (konveyor#589) - Dependency bumps (konveyor#577) - Maven index search config (konveyor#575) Signed-off-by: tsanders <[email protected]>
3 participants
Adapt kantra to use the new organization-centric Cloud Foundry discovery model from asset-generation.
Discovery now groups apps by org rather than space, making
--orgsrequired for discovery mode while--spacesbecomes optional.The
--list-appsnow shows a hierarchical view with organizations at the top level, then spaces, then apps.Fixes: https://issues.redhat.com/browse/MTA-6271
Demo
See demo recording (updated)
(from https://github.com/gciavarrini/demo-kantra-asset-gen)
Signed-off-by: Gloria Ciavarrini [email protected]
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores