-
Notifications
You must be signed in to change notification settings - Fork 43
✨ Support organizations. #484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Jeff Ortel <[email protected]>
WalkthroughAdds a new Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
roles/tackle/templates/customresource-schema.yml.j2 (1)
121-122: Addspacesto the organization's required fields.The organization object currently only requires
name, but an organization without spaces cannot meaningfully filter applications. The past review comment also flagged this issue.Apply this diff to require both
nameandspaces:required: - name + - spacesBased on learnings
🧹 Nitpick comments (2)
roles/tackle/templates/customresource-schema.yml.j2 (2)
102-118: Consider addingminItems: 1to the spaces array.For consistency with the existing Coordinates schema (line 84), which requires at least one space, the
spacesarray in this Filter schema should also specifyminItems: 1to prevent organizations with empty space lists.Apply this diff:
spaces: description: Space definitions within this organization. type: array + minItems: 1 items:
92-124: Consider addingminItemsconstraint to the organizations array.The
organizationsarray currently has no minimum items constraint. An empty organizations array would create a filter that matches nothing, which may not be the intended behavior.Apply this diff if you want to prevent empty organization filters:
organizations: description: A list of organizations, each containing spaces with application names. type: array + minItems: 1 items:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
roles/tackle/templates/customresource-schema.yml.j2(2 hunks)
⏰ 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-operator-bundle
🔇 Additional comments (1)
roles/tackle/templates/customresource-schema.yml.j2 (1)
30-52: LGTM! Well-structured schema with proper migration.The new Coordinates schema version correctly requires all three fields (organization, space, name) with appropriate length constraints. The migration directive properly handles backward compatibility by setting organization to "unknown" for existing data.
jmontleon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
roles/tackle/templates/customresource-schema.yml.j2(2 hunks)
🔇 Additional comments (1)
roles/tackle/templates/customresource-schema.yml.j2 (1)
30-52: LGTM! Migration directive properly handles backward compatibility.The second Coordinates schema version correctly adds the
organizationfield as a required string alongsidespaceandname. The migration directive".organization = \"unknown\""ensures existing data without an organization field will be assigned a default value during schema evolution.
| - definition: | ||
| $schema: https://json-schema.org/draft/2020-12/schema | ||
| title: Coordinates | ||
| type: object | ||
| properties: | ||
| organizations: | ||
| description: Organization names. | ||
| type: array | ||
| items: | ||
| type: string | ||
| minLength: 1 | ||
| spaces: | ||
| description: Space names. | ||
| type: array | ||
| items: | ||
| type: string | ||
| minLength: 1 | ||
| names: | ||
| description: Application names. Each may be a glob expression. | ||
| type: array | ||
| items: | ||
| type: string | ||
| minLength: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Critical: Schema structure doesn't match the described nested hierarchy.
The PR description states this adds support for "organizations with nested spaces and applications" (e.g., Organization "A" has space "Zoo" with applications "Lion" and "Tiger"). However, the current implementation defines three flat, parallel arrays (organizations, spaces, names) with no structural relationship between them.
This flat structure cannot express:
- Which spaces belong to which organizations
- Which applications belong to which spaces
- The hierarchical filtering described in the PR objectives
Based on the past review comments and PR description, the schema should use a nested structure where:
- Each organization object has a
nameand optionalspacesarray - Each space object has a
nameand optionalapplicationsarray
Additionally, no required fields are specified in this schema version. Based on your earlier comment ("Only the root 'organizations' and 'name' fields should be required"), please add:
+ required:
+ - organizationsHowever, since "name" is now inside the names array (not a root field), please clarify whether you meant names should be required at the root level, or if the current flat structure is intentional despite the nested description in the PR objectives.
Would you like me to generate a nested schema structure that matches the PR description, or is the flat structure intentional?
I'll verify this review comment by examining the actual file and its context.
🏁 Script executed:
#!/bin/bash
# Find and display the customresource-schema file with context around lines 87-109
fd -e j2 -e yml 'customresource-schema' --exec cat {} \;Length of output: 2707
🏁 Script executed:
#!/bin/bash
# Search for schema or nested structure definitions in the file
fd -e j2 -e yml 'customresource-schema' --exec head -120 {} \;Length of output: 2707
🏁 Script executed:
#!/bin/bash
# Look for any references to "nested", "organizations", "spaces" in the repository
rg -n "organizations|nested.*spaces|spaces.*applications" --type yaml --type txt -C2Length of output: 3070
🏁 Script executed:
#!/bin/bash
# Check for PR-related files or documentation
fd -e md -e txt 'README|CHANGELOG|PR|description' | head -20Length of output: 24
🏁 Script executed:
#!/bin/bash
# Get git log to find PR description or commit messages
git log --oneline --all -20 | head -20Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Search for any schema definitions with "required" fields
rg -n "required:" --type yaml -A2 -B2Length of output: 62501
🏁 Script executed:
#!/bin/bash
# Look for schema definitions mentioning "Coordinates"
rg -n "Coordinates|schema.*object" --type yaml -A10 -B2Length of output: 1583
🏁 Script executed:
#!/bin/bash
# Check full file structure
fd 'customresource-schema.yml.j2' --exec wc -l {} \;Length of output: 57
🏁 Script executed:
#!/bin/bash
# Display lines around 87-109 with more context
fd 'customresource-schema.yml.j2' --exec sed -n '70,120p' {} \;Length of output: 1103
🏁 Script executed:
#!/bin/bash
# Look for nested structure patterns in yaml templates
rg -n "properties:" --type yaml -A15 | head -80Length of output: 7500
🏁 Script executed:
#!/bin/bash
# Check for array items with nested properties
rg -n "items:" --type yaml -A5 -B2Length of output: 72989
🏁 Script executed:
#!/bin/bash
# Search for "Definition" or filter definitions
rg -n "definition:|Coordinates" --type yaml -B5 -A25Length of output: 56944
🏁 Script executed:
#!/bin/bash
# Get complete context of the schema around lines 87-109
fd 'customresource-schema.yml.j2' --exec sed -n '1,130p' {} \;Length of output: 2707
🏁 Script executed:
#!/bin/bash
# Look for required fields specification
rg -n "^\s+required:" --type yaml -B5 -A3Length of output: 117584
🏁 Script executed:
#!/bin/bash
# Check git diff to see what changed in this PR
git diff HEAD^ HEAD -- roles/tackle/templates/customresource-schema.yml.j2 | head -200Length of output: 2165
🏁 Script executed:
#!/bin/bash
# Look for current branch info
git branch -vLength of output: 122
🏁 Script executed:
#!/bin/bash
# Check PR body or description
git log -1 --format="%B" | head -50Length of output: 58
🏁 Script executed:
#!/bin/bash
# Search for any example or test files showing expected structure
fd -e json -e yaml 'test|example|spec' | xargs grep -l "organizations\|coordinates" 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for filtering logic that might indicate expected structure
rg -n "filter|Filter" --type go --type py --type ts -B2 -A5 | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about the schema
fd -e adoc -e md 'schema\|api\|definition' --exec head -50 {} \;Length of output: 0
🏁 Script executed:
#!/bin/bash
# List all files in the tackle templates directory to understand context
fd '' roles/tackle/templates/ --type f | head -30Length of output: 1575
🏁 Script executed:
#!/bin/bash
# Display the complete customresource-schema.yml.j2 file
cat roles/tackle/templates/customresource-schema.yml.j2Length of output: 2707
🏁 Script executed:
#!/bin/bash
# Look for issue or PR references in commit messages
git log --all --grep="nested\|Coordinates\|organization" --oneline | head -10Length of output: 165
🏁 Script executed:
#!/bin/bash
# Check for any comments or documentation strings in the schema file
grep -n "#\|description\|title" roles/tackle/templates/customresource-schema.yml.j2 | head -50Length of output: 611
🏁 Script executed:
#!/bin/bash
# Look at git history of this specific section
git log -p --follow -- roles/tackle/templates/customresource-schema.yml.j2 | head -300Length of output: 59
🏁 Script executed:
#!/bin/bash
# Search in entire repo for references to nested organizations/spaces/applications structure
rg -n "organization.*space.*application|space.*application|nested" -C3 | head -100Length of output: 404
🏁 Script executed:
#!/bin/bash
# Get current file state directly
if [ -f roles/tackle/templates/customresource-schema.yml.j2 ]; then
echo "=== File exists ===" && wc -l roles/tackle/templates/customresource-schema.yml.j2
echo "=== Lines 1-50 ===" && sed -n '1,50p' roles/tackle/templates/customresource-schema.yml.j2
echo "=== Lines 70-130 ===" && sed -n '70,130p' roles/tackle/templates/customresource-schema.yml.j2
else
echo "File not found"
fiLength of output: 2383
🏁 Script executed:
#!/bin/bash
# Search for additionalProperties or other structural hints
grep -n "additionalProperties\|oneOf\|anyOf\|allOf\|patternProperties" roles/tackle/templates/customresource-schema.yml.j2 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for references to the schema in code that might show how it's used
rg -n "Coordinates|organizations.*spaces|filter" --type go --type py -B3 -A3 | head -150Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for any existing nested structure definitions
grep -n "oneOf\|allOf\|items.*properties\|\$ref" roles/tackle/templates/customresource-schema.yml.j2 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look at git show for current HEAD to see PR state
git show --stat | head -50Length of output: 327
🏁 Script executed:
#!/bin/bash
# List diff for the specific file
git diff HEAD~1..HEAD -- roles/tackle/templates/customresource-schema.yml.j2 2>/dev/null || echo "No git history available"Length of output: 2165
🏁 Script executed:
#!/bin/bash
# Check for Jinja2 filters or template logic that might affect structure
grep -n "{%\|{{\|filter\|loop" roles/tackle/templates/customresource-schema.yml.j2 | head -30Length of output: 219
🏁 Script executed:
#!/bin/bash
# Look for supporting documentation
find . -name "*.md" -o -name "*.adoc" -o -name "*.txt" | xargs grep -l "Coordinates\|schema\|filter" 2>/dev/null | head -10Length of output: 0
Critical: filter schema replaced nested organization→spaces→applications with flat arrays — restore nesting or confirm intentional breaking change
The change in roles/tackle/templates/customresource-schema.yml.j2 (lines ~87–109) removed the nested organization objects and replaced them with parallel arrays (organizations, spaces, names) and omitted required fields. This prevents expressing which spaces/apps belong to which organization and breaks the PR's stated "organizations with nested spaces and applications" behavior.
- Restore the nested schema (organizations: array of objects { name: string, spaces: array of objects { name: string, applications: array[string] } }) so relationships are preserved.
- Re-add appropriate required constraints (at minimum make the root organizations required and ensure application name is required where intended), or explicitly document and implement an alternative model that encodes relationships.
roles/tackle/templates/customresource-schema.yml.j2: lines 87–109.
🤖 Prompt for AI Agents
In roles/tackle/templates/customresource-schema.yml.j2 around lines 87–109 the
schema was flattened to parallel arrays (organizations, spaces, names) which
loses the relationship between org→space→application; revert to a nested schema:
make "organizations" a required property that is an array of objects, each
organization object must require "name" (string, minLength:1) and "spaces"
(array of objects), each space object must require "name" (string, minLength:1)
and "applications" (array of strings, items string minLength:1) so
org→spaces→applications relationships are preserved; ensure required
arrays/properties are re-added at each level and validate types/constraints
match existing conventions in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional. The schema provides for 3 flat optional lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jortel, thank you for the clarification! I understand now that the flat structure with three optional lists is the intended design for the filter schema.
I've noted this for future reference so we don't raise similar concerns about this design choice.
✏️ Learnings added
Learnt from: jortel
Repo: konveyor/operator PR: 484
File: roles/tackle/templates/customresource-schema.yml.j2:87-109
Timestamp: 2025-11-04T17:06:56.256Z
Learning: In roles/tackle/templates/customresource-schema.yml.j2, the cloudfoundry filter schema intentionally uses three flat, optional arrays (organizations, spaces, names) rather than a nested structure. This flat design is the correct implementation despite the PR description mentioning hierarchical organization→spaces→applications relationships.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Resolves: https://issues.redhat.com/browse/MTA-6326 Resolves: konveyor#2709 CloudFoundry schema update: konveyor/operator#484 Update the discover application wizard CloudFoundry source platform filter input and review to support organizations, spaces, and names. Added the ability to validate that two JSON schemas are functionally equivalent. This is used to validate that the Cloud Foundry schema is functionally equivalent to the schema used to build the Cloud Foundry forms. If a source platform is kind of "cloudfoundry", and the filter schema pulled from hub is functionally equivalent to the schema the components are based on, then show the custom components. If the schema is not equivalent, then show the generic schema defined fields components. This will most likely give the user a code editor to input a json document to define the filter. Signed-off-by: Scott J Dickerson <[email protected]>
Resolves: https://issues.redhat.com/browse/MTA-6326 Resolves: #2709 CloudFoundry schema update: konveyor/operator#484 ## Summary Update the discover application wizard CloudFoundry source platform filter input and review to support organizations, spaces, and names. Added the ability to validate that two JSON schemas are functionally equivalent. This is used to validate that the Cloud Foundry schema is functionally equivalent to the schema used to build the Cloud Foundry forms. If a source platform is kind of "cloudfoundry", and the filter schema pulled from hub is functionally equivalent to the schema the components are based on, then show the custom components. If the schema is not equivalent, then show the generic schema defined fields components. This will most likely give the user a code editor to input a json document to define the filter. ## Summary by CodeRabbit * **New Features** * Added Cloud Foundry discovery filters: separate Organizations, Spaces, and Names in import and review flows. * **Chores** * Updated dependency declarations and added lodash-es integration. * Moved and consolidated JSON Schema utilities for clearer schema handling and validation. * **Tests** * Expanded JSON Schema utility test coverage and removed tests for a deprecated combine helper. --------- Signed-off-by: Scott J Dickerson <[email protected]>
Resolves: https://issues.redhat.com/browse/MTA-6326 Resolves: konveyor#2709 CloudFoundry schema update: konveyor/operator#484 Update the discover application wizard CloudFoundry source platform filter input and review to support organizations, spaces, and names. Added the ability to validate that two JSON schemas are functionally equivalent. This is used to validate that the Cloud Foundry schema is functionally equivalent to the schema used to build the Cloud Foundry forms. If a source platform is kind of "cloudfoundry", and the filter schema pulled from hub is functionally equivalent to the schema the components are based on, then show the custom components. If the schema is not equivalent, then show the generic schema defined fields components. This will most likely give the user a code editor to input a json document to define the filter. * **New Features** * Added Cloud Foundry discovery filters: separate Organizations, Spaces, and Names in import and review flows. * **Chores** * Updated dependency declarations and added lodash-es integration. * Moved and consolidated JSON Schema utilities for clearer schema handling and validation. * **Tests** * Expanded JSON Schema utility test coverage and removed tests for a deprecated combine helper. --------- Signed-off-by: Scott J Dickerson <[email protected]>
) (#2722) Resolves: https://issues.redhat.com/browse/MTA-6326 Resolves: #2709 Backport-Of: #2718 CloudFoundry schema update: konveyor/operator#484 ## Summary Update the discover application wizard CloudFoundry source platform filter input and review to support organizations, spaces, and names. Added the ability to validate that two JSON schemas are functionally equivalent. This is used to validate that the Cloud Foundry schema is functionally equivalent to the schema used to build the Cloud Foundry forms. If a source platform is kind of "cloudfoundry", and the filter schema pulled from hub is functionally equivalent to the schema the components are based on, then show the custom components. If the schema is not equivalent, then show the generic schema defined fields components. This will most likely give the user a code editor to input a json document to define the filter. ## Summary by CodeRabbit * **New Features** * Added Cloud Foundry discovery filters: separate Organizations, Spaces, and Names in import and review flows. * **Chores** * Updated dependency declarations and added lodash-es integration. * Moved and consolidated JSON Schema utilities for clearer schema handling and validation. * **Tests** * Expanded JSON Schema utility test coverage and removed tests for a deprecated combine helper. Signed-off-by: Scott J Dickerson <[email protected]>
…ma (konveyor#491) Backport-Of: konveyor#491 Backport-Of: konveyor#484 See: konveyor/tackle2-ui#2726 For live discovery, at least one organization name is required. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Chores** * Enhanced data validation to enforce minimum array requirements for improved system reliability and robustness. Array fields and organizations now require at least one item minimum, preventing incomplete configurations and improving overall data consistency. These updates ensure more reliable and robust system behavior when managing resources and organizational structures throughout the application. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Scott J Dickerson <[email protected]>
Add organizations to the filter and organization to the coordinates. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Expanded Cloud Foundry coordinates: now include organization, space, and app name (all required); migration defaults unknown organizations to "unknown". * Added Cloud Foundry filter model to support filtering by organizations → spaces → applications. * **Chores** * CI lint step simplified by disabling Python setup in the workflow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jeff Ortel <[email protected]>
…ma (#491) (#494) Backport-Of: #491 Backport-Of: #484 See: konveyor/tackle2-ui#2726 For live discovery, at least one organization name is required. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary * **Chores** * Enhanced data validation to enforce minimum array requirements for improved system reliability and robustness. Array fields and organizations now require at least one item minimum, preventing incomplete configurations and improving overall data consistency. These updates ensure more reliable and robust system behavior when managing resources and organizational structures throughout the application. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jeff Ortel <[email protected]> Signed-off-by: Scott J Dickerson <[email protected]> Co-authored-by: Jeff Ortel <[email protected]>
Add organizations to the filter and organization to the coordinates.
Summary by CodeRabbit
New Features
Chores