Sync ODH main with upstream v0.3.5 #1110
Conversation
Signed-off-by: ppadti <[email protected]>
…ment mode (kubeflow#2056) * feat: enable standalone UI in docker-compose with configurable deployment mode Signed-off-by: Eder Ignatowicz <[email protected]> * fix(ui): address PR review - separate standalone Dockerfile Address PR review feedback from @lucferbux: - Create dedicated Dockerfile.standalone with envtest support to avoid bloating production image with K8s test binaries (~150MB) - Keep main Dockerfile clean for kubeflow/federated deployments - Update docker-compose.yaml to use ui-standalone image - Add explicit --mock-mr-client=false and --mock-mr-catalog-client=false flags for better readability - Update Makefile and GitHub workflow to use new Dockerfile - Document Dockerfile structure in README Signed-off-by: Eder Ignatowicz <[email protected]> --------- Signed-off-by: Eder Ignatowicz <[email protected]>
📝 WalkthroughWalkthroughIntroduces a standalone Dockerfile with multi-stage builds for the UI component (Node frontend + Go BFF backend), adds environment variable support to the main UI Dockerfile, enhances mock Kubernetes testing with configurable asset directories, expands mock data with comprehensive filter options and performance metrics, updates artifact filter query generation logic, and provisions the UI service in Docker Compose files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![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
🤖 Fix all issues with AI agents
In @docker-compose-local.yaml:
- Around line 35-36: The UI service in docker-compose-local.yaml is restricted
to the "postgres" profile, preventing users running the "mysql" profile from
starting the UI; update the UI service's profiles list so it includes both
"postgres" and "mysql" (or remove the profiles key to make it always available),
ensuring consistency with model-registry and model-catalog which support both
backends and keeping the UI database-agnostic.
🧹 Nitpick comments (3)
clients/ui/bff/internal/integrations/kubernetes/k8mocks/mock_factory.go (1)
82-92: Consider clarifying the error fallback behavior.The implementation correctly provides a default test user identity when
ExtractRequestIdentityfails, enabling RBAC tests without real authentication headers. However, the comment suggests this only handles "missing headers," but the code catches all errors from the underlying factory (validation failures, malformed tokens, etc.).Suggested improvements
Option 1: Clarify the comment to reflect actual behavior
func (f *MockedStaticClientFactory) ExtractRequestIdentity(httpHeader http.Header) (*k8s.RequestIdentity, error) { identity, err := f.realFactoryWithoutClient.ExtractRequestIdentity(httpHeader) if err != nil { - // In mock mode, use default test user when header is missing + // In mock mode, use default test user when identity extraction fails (missing/invalid headers) return &k8s.RequestIdentity{ UserID: DefaultTestUsers[0].UserName, Groups: DefaultTestUsers[0].Groups, }, nil } return identity, nil }Option 2: Add debug logging for observability
func (f *MockedStaticClientFactory) ExtractRequestIdentity(httpHeader http.Header) (*k8s.RequestIdentity, error) { identity, err := f.realFactoryWithoutClient.ExtractRequestIdentity(httpHeader) if err != nil { + f.logger.Debug("identity extraction failed, using default test user", slog.String("error", err.Error())) // In mock mode, use default test user when header is missing return &k8s.RequestIdentity{ UserID: DefaultTestUsers[0].UserName, Groups: DefaultTestUsers[0].Groups, }, nil } return identity, nil }clients/ui/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go (1)
51-73: LGTM! Environment-driven binary assets configuration works well.The implementation correctly supports both containerized environments (via
ENVTEST_ASSETS_DIR) and local development (via project root detection). The path construction is cross-platform compatible usingfilepath.Join, and the fallback logic preserves existing behavior.Optional: Extract Kubernetes version to constant
The hardcoded version
"1.29.0"appears in both branches. Consider extracting it to a package-level constant for easier maintenance:+const EnvTestK8sVersion = "1.29.0" + func SetupEnvTest(input TestEnvInput) (*envtest.Environment, kubernetes.Interface, error) { var binaryAssetsDir string // Check for explicit envtest assets directory (used in Docker) if envDir := os.Getenv("ENVTEST_ASSETS_DIR"); envDir != "" { // Construct full path with OS/ARCH suffix binaryAssetsDir = filepath.Join(envDir, "k8s", - fmt.Sprintf("1.29.0-%s-%s", runtime.GOOS, runtime.GOARCH)) + fmt.Sprintf("%s-%s-%s", EnvTestK8sVersion, runtime.GOOS, runtime.GOARCH)) } else { // Fall back to project root detection (local development) projectRoot, err := getProjectRoot() if err != nil { input.Logger.Error("failed to find project root", slog.String("error", err.Error())) input.Cancel() os.Exit(1) } binaryAssetsDir = filepath.Join(projectRoot, "bin", "k8s", - fmt.Sprintf("1.29.0-%s-%s", runtime.GOOS, runtime.GOARCH)) + fmt.Sprintf("%s-%s-%s", EnvTestK8sVersion, runtime.GOOS, runtime.GOARCH)) }clients/ui/Dockerfile.standalone (1)
50-52: Consider pinning the setup-envtest version more precisely.The current command uses
@release-0.17which tracks a release branch. For reproducible builds, consider pinning to a specific version tag (e.g.,@v0.17.0) to prevent unexpected behavior changes when new commits are added to the release branch.♻️ Suggested improvement
# Install setup-envtest and download K8s binaries for mock mode -RUN go install sigs.k8s.io/controller-runtime/tools/[email protected] +RUN go install sigs.k8s.io/controller-runtime/tools/[email protected] RUN setup-envtest use 1.29.0 --bin-dir /envtest-bin -p path
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/build-and-push-ui-images-standalone.ymlclients/ui/Dockerfileclients/ui/Dockerfile.standaloneclients/ui/Makefileclients/ui/README.mdclients/ui/bff/internal/integrations/kubernetes/k8mocks/base_testenv.goclients/ui/bff/internal/integrations/kubernetes/k8mocks/mock_factory.goclients/ui/bff/internal/mocks/static_data_mock.goclients/ui/frontend/src/app/pages/modelCatalog/utils/modelCatalogUtils.tsdocker-compose-local.yamldocker-compose.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
clients/ui/bff/internal/integrations/kubernetes/k8mocks/mock_factory.go (2)
clients/ui/bff/internal/integrations/kubernetes/types.go (1)
RequestIdentity(13-17)clients/ui/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go (1)
DefaultTestUsers(20-36)
clients/ui/frontend/src/app/pages/modelCatalog/utils/modelCatalogUtils.ts (2)
clients/ui/frontend/src/app/modelCatalogTypes.ts (1)
CatalogFilterOptions(239-244)clients/ui/frontend/src/concepts/modelCatalog/const.ts (2)
LatencyMetricFieldName(29-29)ALL_LATENCY_FIELD_NAMES(59-63)
clients/ui/bff/internal/mocks/static_data_mock.go (1)
clients/ui/bff/internal/models/catalog_model_list.go (2)
FilterOption(37-41)FilterRange(32-35)
⏰ 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). (13)
- GitHub Check: build-and-test-csi-image
- GitHub Check: build
- GitHub Check: prepare / prepare
- GitHub Check: build-and-test-image
- GitHub Check: build-image
- GitHub Check: Run on Ubuntu
- GitHub Check: E2E Py 3.12 K8s v1.33.7 DB db
- GitHub Check: E2E Py 3.12 K8s v1.33.7 DB postgres
- GitHub Check: E2E Py 3.11 K8s v1.33.7 DB db
- GitHub Check: E2E Py 3.10 K8s v1.33.7 DB db
- GitHub Check: test-and-build
- GitHub Check: py-test
- GitHub Check: job-test
🔇 Additional comments (17)
clients/ui/frontend/src/app/pages/modelCatalog/utils/modelCatalogUtils.ts (4)
273-292: LGTM!The
findServerFilterKeyhelper correctly resolves local filter IDs to their server-side keys, handling both direct matches andartifacts.{filterId}{suffix}pattern variations.
294-303: LGTM!Both helper functions are well-implemented. The
isLatencyFieldNametype guard using.some()is appropriate here as it avoids type casting issues that.includes()would require.
315-342: LGTM!The refactored filter logic correctly:
- Filters to only artifact-relevant fields (hardware type, use case, latency metrics)
- Resolves server-side keys for proper filter option lookup
- Converts to query keys for the output filter string
345-351: Verify the intentional difference in filter query formatting.The latency filter query here uses no spaces around the operator (
${queryKey}<${data}), whilefiltersToFilterQueryat line 254 uses spaces (${filterId} < ${data}). If this is intentional due to different backend parsing requirements, a comment explaining the difference would help maintainability.clients/ui/bff/internal/mocks/static_data_mock.go (3)
1685-1690: Verify the"<nil>"string value is intentional.The value
"<nil>"appears to be a string representation of a nil value rather than an actual data value. If this is meant to represent an absent/null dataset, consider either:
- Omitting this filter option entirely
- Using an empty string or a more meaningful placeholder
691-691: LGTM - Provider name anonymized for mock data.The change from "Red Hat" to "Provider one" aligns with the anonymization pattern used elsewhere in the mock data (e.g., "provider1", "provider2").
1528-2033: LGTM - Comprehensive filter options expansion.The expanded filter options provide thorough mock data coverage for:
- Model metadata (status, model_type, size, tensor_type)
- Validation information (validated_on, variant_group_id)
- Artifact properties (use_case, hardware_type, hardware_configuration, etc.)
- Performance metrics (TTFT, E2E, ITL, TPS ranges)
- Benchmark scores (AIME, ARC, BBH, GPQA, etc.)
The filter options correctly use the
FilterRangeandFilterOptiontypes from the models package.clients/ui/Dockerfile (1)
13-25: LGTM - Build-time configuration for deployment mode and theme.The Dockerfile now properly accepts
DEPLOYMENT_MODEandSTYLE_THEMEas build arguments and passes them to the frontend build. This enables consistent configuration across different deployment targets (kubeflow, standalone, federated)..github/workflows/build-and-push-ui-images-standalone.yml (1)
70-84: LGTM - Explicit Dockerfile path for standalone build.The workflow now correctly specifies
file: ./clients/ui/Dockerfile.standaloneto ensure the standalone image is built from the dedicated Dockerfile. The build arguments and attestation settings are appropriately configured.clients/ui/Makefile (1)
81-95: LGTM - Makefile targets updated to use Dockerfile.standalone.Both
docker-build-standaloneanddocker-buildx-standalonetargets now correctly specify-f Dockerfile.standaloneto use the dedicated standalone Dockerfile. This aligns with the workflow and docker-compose configurations.clients/ui/README.md (1)
122-130: LGTM - Clear documentation of Dockerfile structure.The new section clearly explains the purpose of each Dockerfile:
Dockerfile: Production image for Kubeflow/federated deploymentsDockerfile.standalone: Development image with K8s test binaries for mock modeThis helps developers understand which Dockerfile to use for different scenarios.
clients/ui/Dockerfile.standalone (2)
1-68: LGTM - Well-structured multi-stage Dockerfile for standalone deployment.The Dockerfile follows best practices:
- Multi-stage build separates UI and BFF compilation
- Distroless final image minimizes attack surface
- Non-root user (65532:65532) for security
- CGO disabled for static binary
- Envtest binaries included for K8s mock mode support
7-7: No action required. Thegolang:1.24.6base image is a valid and available Docker tag. Go 1.24 has been released, and the specified version is legitimate.docker-compose.yaml (2)
14-34: Consider adding the UI service to the MySQL profile as well.The
model-registry-uiservice is only available under thepostgresprofile. If users running with MySQL also need the UI, consider addingmysqlto the profiles list or removing the profile restriction entirely.Is the postgres-only profile restriction intentional? If not:
♻️ Suggested change to support both profiles
profiles: - postgres + - mysql
17-24: LGTM - Standalone UI service configuration.The command flags are well-configured:
--mock-k8s-client=true: Uses envtest binaries for K8s mocking--mock-mr-client=false/--mock-mr-catalog-client=false: Connects to real model-registry and model-catalog services--dev-mode=true/--deployment-mode=standalone: Appropriate for local development--log-level=DEBUG: Helpful for development debuggingdocker-compose-local.yaml (2)
14-20: LGTM! Build configuration aligns with standalone UI objectives.The build configuration correctly uses
Dockerfile.standaloneand appropriate build arguments for standalone deployment mode.
21-34: Clarify the service connectivity mechanism in dev-mode.With
--dev-mode=true, the UI uses hardcoded localhost with default ports (8080 for model-registry, 8081 for model-catalog) rather than Docker Compose service DNS. This works because:
- Port mappings expose
8080:8080and8081:8081on both services- The UI container has
extra_hosts: localhost:host-gatewayto reach these ports- The dev-mode flag in
clients/ui/bff/internal/api/middleware.gooverrides service discovery to usehttp://localhost:PORT/api/...instead of Kubernetes ClusterIP resolutionConnectivity is functional but implicit—the defaults must match the docker-compose ports. If port assignments change, the UI flag
--dev-mode-model-registry-portand--dev-mode-catalog-portshould be updated accordingly.
| profiles: | ||
| - postgres |
There was a problem hiding this comment.
UI restricted to postgres profile only.
The UI service is limited to the postgres profile, meaning users running the mysql profile won't have access to the UI. Since the model-registry and model-catalog services support both database backends, consider adding the UI to both profiles or making it database-agnostic.
🔧 Suggested fix to support both profiles
profiles:
- postgres
+ - mysql📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| profiles: | |
| - postgres | |
| profiles: | |
| - postgres | |
| - mysql |
🤖 Prompt for AI Agents
In @docker-compose-local.yaml around lines 35 - 36, The UI service in
docker-compose-local.yaml is restricted to the "postgres" profile, preventing
users running the "mysql" profile from starting the UI; update the UI service's
profiles list so it includes both "postgres" and "mysql" (or remove the profiles
key to make it always available), ensuring consistency with model-registry and
model-catalog which support both backends and keeping the UI database-agnostic.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonburdo, pboyd, syntaxsdev The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 4d40565
into
opendatahub-io:main
5 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.