Inline editor component #17
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ogs of a component
…s and context handling; add PluginConditionEvaluator for extension filtering
…mproved clarity and performance; add evaluateSingleCondition for condition evaluation
- Moved `choreo-context` from plugins to libs for better organization. - Updated `pnpm-lock.yaml` to reflect new paths and added dependencies for the `choreo-context` library. - Modified `rush.json` to point to the new project folder for `choreo-context`. - Updated imports in various files to accommodate the new structure. - Implemented hooks for client, color mode, global state, and organizations. - Created context providers for API client and global state management. - Added reducers for application state management. - Removed old plugin structure and integrated necessary components into the new library structure. - Added TypeScript configuration and ESLint setup for the new library.
…tructure and basePath handling
…and navigation - Implemented ComponentListPage with loading and error handling. - Created ComponentListPanel for displaying project components. - Developed ProjectListPanel for managing project lists with search functionality. - Added ComponentSummary to summarize component details. - Integrated navigation items for accessing components. - Defined plugin manifest for resource listing with all extensions.
- Removed deprecated overview plugin components and their associated files. - Updated the plugin registry to include projectListing and componentListing. - Corrected spelling of 'extensionPoint' in multiple locations. - Consolidated organization and project overview components into a new structure. - Ensured backward compatibility by maintaining necessary exports.
…sePath from IAppState
… context and add URL parameter hooks
…er clarity and performance
…s and context handling; add PluginConditionEvaluator for extension filtering
…mproved clarity and performance; add evaluateSingleCondition for condition evaluation
…improved clarity and consistency
…aluateWhenExpression functions
…logic for clarity
…adability and maintainability
…d organization references
…r/openchoreo into rendering-conditionally
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces multiple Helm charts for deploying OpenChoreo components across different architectural planes (Control Plane, Data Plane, Build Plane, Identity Provider, and Observability Plane). The PR also removes outdated documentation and samples while updating CRD definitions to consolidate resources and improve the overall architecture.
Key changes include:
- New Helm charts for all OpenChoreo planes with comprehensive configuration options
- Removal of legacy v2 CRDs (BuildV2, ComponentV2, EndpointV2, EndpointClass) in favor of consolidated resources
- Enhanced status tracking for bindings and releases with endpoint information
- Updated workflow templates for the build plane with registry configuration options
Reviewed Changes
Copilot reviewed 211 out of 516 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| install/helm/openchoreo-identity-provider/* | New Helm chart for identity provider with database persistence and post-install hooks |
| install/helm/openchoreo-data-plane/values.yaml | Updated logging paths and OpenSearch hostname configuration |
| install/helm/openchoreo-control-plane/* | Updated CRDs, removed legacy resources, added new component classes |
| install/helm/openchoreo-build-plane/* | New workflow templates and registry configuration options |
| install/check-status.sh | Complete rewrite with improved component grouping and status visualization |
| Various CRD files | Consolidated endpoint types, removed operations field, updated connection specifications |
Files not reviewed (1)
- choreo-ui/common/config/rush/pnpm-lock.yaml: Language not supported
| restartPolicy: Never | ||
| containers: | ||
| - name: post-install-setup | ||
| image: curlimages/curl:8.4.0 |
There was a problem hiding this comment.
The curl image version 8.4.0 may have known security vulnerabilities. Consider using the latest version or a more recent stable version with security patches.
Suggested change
| image: curlimages/curl:8.4.0 | |
| image: curlimages/curl:8.8.0 |
Comment on lines
35
to
36
| runAsUser: 0 | ||
| runAsGroup: 0 |
There was a problem hiding this comment.
Running the init container as root (runAsUser: 0) poses security risks. Consider using a non-root user or implementing proper capability management instead of running as root.
Suggested change
| runAsUser: 0 | |
| runAsGroup: 0 | |
| runAsUser: 1000 | |
| runAsGroup: 1000 |
| LANG="{{ "{{" }}workflow.parameters.language{{ "}}" }}" | ||
| LANG_VER="{{ "{{" }}workflow.parameters.language-version{{ "}}" }}" | ||
| BUILDER="gcr.io/buildpacks/builder@sha256:5977b4bd47d3e9ff729eefe9eb99d321d4bba7aa3b14986323133f40b622aef1" |
There was a problem hiding this comment.
The hardcoded SHA256 digest for the builder image makes updates difficult. Consider using a configurable parameter or at least a comment explaining why this specific digest is required.
Suggested change
| BUILDER="gcr.io/buildpacks/builder@sha256:5977b4bd47d3e9ff729eefe9eb99d321d4bba7aa3b14986323133f40b622aef1" | |
| # The builder image is pinned to a specific digest for reproducibility and security. | |
| # To update, change the value of 'googleBuildpacks.builderImage' in your values.yaml. | |
| BUILDER="{{ .Values.googleBuildpacks.builderImage }}" |
| else | ||
| echo "Cloning branch: $BRANCH with latest commit" | ||
| git clone --single-branch --branch $BRANCH --depth 1 "REPO" /mnt/vol/source | ||
| git clone --single-branch --branch $BRANCH --depth 1 "$REPO" /mnt/vol/source |
There was a problem hiding this comment.
There's an inconsistency in the git clone command. Line 59 uses 'REPO' while line 65 uses '$REPO'. This could cause the clone to fail when REPO is not defined as an environment variable.
Comment on lines
173
to
175
| printf "\n%b%b╔══════════════════════════════════════════════════════════════════════╗%b\n" "$BOLD" "$BLUE" "$RESET" | ||
| printf "%b%b║ OpenChoreo Component Status ║%b\n" "$BOLD" "$BLUE" "$RESET" | ||
| printf "%b%b╚══════════════════════════════════════════════════════════════════════╝%b\n" "$BOLD" "$BLUE" "$RESET" |
There was a problem hiding this comment.
[nitpick] The hardcoded border width (70 characters) could cause alignment issues if component names or status text exceed expected lengths. Consider making the border width dynamic or adding validation for content width.
Suggested change
| printf "\n%b%b╔══════════════════════════════════════════════════════════════════════╗%b\n" "$BOLD" "$BLUE" "$RESET" | |
| printf "%b%b║ OpenChoreo Component Status ║%b\n" "$BOLD" "$BLUE" "$RESET" | |
| printf "%b%b╚══════════════════════════════════════════════════════════════════════╝%b\n" "$BOLD" "$BLUE" "$RESET" | |
| # Dynamically determine the box width | |
| local title="OpenChoreo Component Status" | |
| local min_width=70 | |
| local max_content_width=${#title} | |
| # Calculate max width of group names and component lines | |
| for group in "${group_order[@]}"; do | |
| group_display_name=$(get_group_display_name "$group") | |
| if (( ${#group_display_name} > max_content_width )); then | |
| max_content_width=${#group_display_name} | |
| fi | |
| components_str=$(get_component_group "$group") | |
| read -r -a components <<< "$components_str" | |
| for component in "${components[@]}"; do | |
| # Compose a sample line as it would be printed (icon + name + status) | |
| status=$(get_component_status "$component" "$context") | |
| icon=$(get_status_icon "$status") | |
| status_color=$(get_status_color "$status") | |
| status_text="$status" | |
| line_length=$(( 2 + 1 + ${#component} + 3 + ${#status_text} )) # icon + space + name + " - " + status | |
| if (( line_length > max_content_width )); then | |
| max_content_width=$line_length | |
| fi | |
| done | |
| done | |
| # Add padding for aesthetics | |
| local padding=4 | |
| local box_width=$(( max_content_width + padding )) | |
| if (( box_width < min_width )); then | |
| box_width=$min_width | |
| fi | |
| # Build border lines | |
| local border_top="╔$(printf '═%.0s' $(seq 1 $box_width))╗" | |
| local border_bottom="╚$(printf '═%.0s' $(seq 1 $box_width))╝" | |
| # Center the title | |
| local title_pad_left=$(( (box_width - ${#title}) / 2 )) | |
| local title_pad_right=$(( box_width - ${#title} - title_pad_left )) | |
| local title_line="║$(printf ' %.0s' $(seq 1 $title_pad_left))$title$(printf ' %.0s' $(seq 1 $title_pad_right))║" | |
| printf "\n%b%b%s%b\n" "$BOLD" "$BLUE" "$border_top" "$RESET" | |
| printf "%b%b%s%b\n" "$BOLD" "$BLUE" "$title_line" "$RESET" | |
| printf "%b%b%s%b\n" "$BOLD" "$BLUE" "$border_bottom" "$RESET" |
…uginConditionEvaluator
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Approach
Related Issues
Checklist
Remarks