periodic sync upstream KF to midstream ODH#244
periodic sync upstream KF to midstream ODH#244openshift-merge-bot[bot] merged 7 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: Adysen Rothman <85646824+adysenrothman@users.noreply.github.com>
* Add api tests stateless Signed-off-by: fege <fmosca@redhat.com> * Fix github action failures, lint and nox Signed-off-by: fege <fmosca@redhat.com> * Add too_slow and remove code to force the model version id Signed-off-by: fege <fmosca@redhat.com> * Add filter_too_much Signed-off-by: fege <fmosca@redhat.com> * add hook to avoid Unsatisfiable Signed-off-by: fege <fmosca@redhat.com> * remove variable Signed-off-by: fege <fmosca@redhat.com> * add artifact_states Signed-off-by: fege <fmosca@redhat.com> * Add example to schema Signed-off-by: fege <fmosca@redhat.com> * Add example in src Signed-off-by: fege <fmosca@redhat.com> * register strategy for string of int64 Signed-off-by: fege <fmosca@redhat.com> * sort imports Signed-off-by: fege <fmosca@redhat.com> * modify case for problematic endpoints Signed-off-by: fege <fmosca@redhat.com> * add more examples Signed-off-by: fege <fmosca@redhat.com> * pin urllib Signed-off-by: fege <fmosca@redhat.com> * exclude problematic endpoints and test with valid data Signed-off-by: fege <fmosca@redhat.com> * Add gha to run the test and mark them with fuzz Signed-off-by: fege <fmosca@redhat.com> * revert change in Makefile pushed by error Signed-off-by: fege <fmosca@redhat.com> * skip test not marked with e2e or fuzz, trigger the fuzz on pr comment Signed-off-by: fege <fmosca@redhat.com> * trigger with label Signed-off-by: fege <fmosca@redhat.com> * correct the label name Signed-off-by: fege <fmosca@redhat.com> * add types and semplify the if Signed-off-by: fege <fmosca@redhat.com> * do not run e2e if test-fuzz label is added Signed-off-by: fege <fmosca@redhat.com> * unpin urllib Signed-off-by: fege <fmosca@redhat.com> * modify lock Signed-off-by: fege <fmosca@redhat.com> * fix: remove proc on label Signed-off-by: Alessio Pragliola <seth.pro@gmail.com> --------- Signed-off-by: fege <fmosca@redhat.com> Signed-off-by: Alessio Pragliola <seth.pro@gmail.com> Co-authored-by: Alessio Pragliola <seth.pro@gmail.com>
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
* Add state machine tests Signed-off-by: fege <fmosca@redhat.com> * adjust if to test on the open PR Signed-off-by: fege <fmosca@redhat.com> * revert if and remove debugging prints Signed-off-by: fege <fmosca@redhat.com> * revert if and remove debugging prints Signed-off-by: fege <fmosca@redhat.com> * Remove db-cleanup Signed-off-by: fege <fmosca@redhat.com> --------- Signed-off-by: fege <fmosca@redhat.com>
…ontend (kubeflow#1261) Bumps [ts-jest](https://github.com/kulshekhar/ts-jest) from 29.3.4 to 29.4.0. - [Release notes](https://github.com/kulshekhar/ts-jest/releases) - [Changelog](https://github.com/kulshekhar/ts-jest/blob/main/CHANGELOG.md) - [Commits](kulshekhar/ts-jest@v29.3.4...v29.4.0) --- updated-dependencies: - dependency-name: ts-jest dependency-version: 29.4.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: lucferbux <lferrnan@redhat.com>
Resolved conflict in .github/workflows/python-tests.yml by adding the fuzz test step in job here in m/s Signed-off-by: Matteo Mortari <matteo.mortari@gmail.com>
WalkthroughThis update introduces fuzz and property-based testing for the Python Model Registry client, enhances test infrastructure, and adds Schemathesis integration. The UI and backend are refactored to use a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CI
participant Make as Makefile/Nox
participant PyTest as Pytest/Schemathesis
participant SUT as Model Registry API
Dev->>Make: make test-fuzz
Make->>Make: Deploy services (Minio, Registry, etc.)
Make->>Make: Install dependencies (Poetry, Schemathesis)
Make->>PyTest: Run pytest --fuzz
PyTest->>SUT: Fuzz API requests (Schemathesis)
SUT-->>PyTest: API responses
PyTest->>Make: Report test results
Make->>Make: Cleanup (DB, artifacts)
sequenceDiagram
participant User as User
participant UI as Model Registry UI
participant NGINX as Auth Proxy (Standalone)
participant BFF as Backend For Frontend
participant API as Model Registry API
User->>UI: Access UI (port 8080)
UI->>NGINX: Request (port 8000)
NGINX->>BFF: Proxy with kubeflow-userid header
BFF->>API: API call with auth context
API-->>BFF: Response
BFF-->>NGINX: Response
NGINX-->>UI: Response
UI-->>User: Rendered page
Suggested labels
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
Bot
added
Area/UI
Area/MR Python client
Area/GitHub
Area/Manifests
labels
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tarilabs 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tarilabs 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
clients/python/tests/test_api_stateful.py (1)
5-17: LGTM - Good fuzz testing implementation with minor formatting issueThe stateful fuzz testing implementation looks solid. The hypothesis settings are appropriate for API testing, limiting examples and deadline while suppressing relevant health checks.
However, there's a formatting issue with the inline comment on line 11.
Apply this diff to fix the comment formatting:
- deadline=10000, #10 seconds + deadline=10000, # 10 secondsmanifests/kustomize/options/ui/overlays/standalone/model-registry-ui-deployment.yaml (1)
48-48: Add missing newline at end of file.The YAML file should end with a newline character to follow standard formatting conventions.
emptyDir: {} +clients/ui/bff/internal/repositories/model_registry.go (1)
21-21: TODO reminder for future enhancementThe TODO comment indicates planned functionality for fetching Routes for external access in default mode.
Would you like me to create an issue to track implementing Route-based external access for the default deployment mode?
manifests/kustomize/options/ui/overlays/standalone/auth-proxy-configmap.yaml (1)
1-53: Fix YAML formatting issuesThe file has trailing spaces and is missing a newline at the end of file.
Apply this fix to address the formatting issues:
pid /tmp/nginx.pid; error_log /dev/stderr; - + events { worker_connections 1024; } - + http { include /etc/nginx/mime.types; default_type application/octet-stream; - + access_log /dev/stdout; - + # Use /tmp for cache directories to avoid permission issues client_body_temp_path /tmp/client_temp; proxy_temp_path /tmp/proxy_temp; fastcgi_temp_path /tmp/fastcgi_temp; uwsgi_temp_path /tmp/uwsgi_temp; scgi_temp_path /tmp/scgi_temp; - + upstream backend { server localhost:8080; } - + server { listen 8000; server_name _; - + location /login { return 200 '{"message": "Login successful", "kubeflow-userid": "user@example.com"}'; add_header Content-Type application/json; } - + location / { # Always inject kubeflow-userid header proxy_pass http://backend; proxy_set_header kubeflow-userid "user@example.com"; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header Connection ""; proxy_http_version 1.1; proxy_buffering off; } } - } + } +clients/ui/bff/internal/integrations/kubernetes/internal_k8s_client.go (1)
144-144: Consider making worker pool size configurableThe fixed worker pool size of 10 is reasonable, but for larger clusters with many namespaces, it might be beneficial to make this configurable.
Consider adding a configuration option or calculating based on namespace count:
-const numWorkers = 10 // Fixed number of workers to avoid resource overhead +// Calculate workers based on namespace count, capped at a reasonable maximum +numWorkers := len(namespaceList.Items) / 10 +if numWorkers < 5 { + numWorkers = 5 +} else if numWorkers > 20 { + numWorkers = 20 +}clients/python/tests/test_api.py (2)
38-42: Complete the docstring.The docstring is incomplete and should describe what the test validates.
Apply this diff to improve the docstring:
-def test_mr_api_stateless(auth_headers: dict, case: schemathesis.Case): - """Test the Model Registry API endpoints. - - This test uses schemathesis to generate and validate API requests - """ +def test_mr_api_stateless(auth_headers: dict, case: schemathesis.Case): + """Test the Model Registry API endpoints using property-based testing. + + This test uses schemathesis to generate and validate API requests + against the OpenAPI schema, excluding artifact PATCH and model version + artifacts POST endpoints which are tested separately. + """
52-54: Verify model version existence before creating artifacts.The test uses a randomly generated
model_version_idwithout verifying if it exists. This could lead to inconsistent test behavior if the API validates the existence of the model version.Consider either:
- Creating a model version first and using its ID
- Using a known/fixed model version ID from test fixtures
- Documenting the expected behavior when the model version doesn't exist
Would you like me to help implement a fixture that creates a model version for testing?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clients/python/poetry.lockis excluded by!**/*.lockclients/ui/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
.github/workflows/python-tests.yml(1 hunks)README.md(1 hunks)clients/python/.gitignore(1 hunks)clients/python/Makefile(2 hunks)clients/python/README.md(1 hunks)clients/python/noxfile.py(3 hunks)clients/python/pyproject.toml(2 hunks)clients/python/schemathesis.toml(1 hunks)clients/python/tests/conftest.py(3 hunks)clients/python/tests/constants.py(1 hunks)clients/python/tests/test_api.py(1 hunks)clients/python/tests/test_api_stateful.py(1 hunks)clients/python/tests/test_client.py(0 hunks)clients/ui/.env(1 hunks)clients/ui/.env.production(1 hunks)clients/ui/Dockerfile(1 hunks)clients/ui/Makefile(3 hunks)clients/ui/README.md(3 hunks)clients/ui/bff/Makefile(2 hunks)clients/ui/bff/cmd/main.go(2 hunks)clients/ui/bff/internal/api/app.go(1 hunks)clients/ui/bff/internal/api/middleware.go(1 hunks)clients/ui/bff/internal/api/model_registry_settings_groups_handler_test.go(1 hunks)clients/ui/bff/internal/api/model_registry_settings_rbac_handler_test.go(1 hunks)clients/ui/bff/internal/config/environment.go(3 hunks)clients/ui/bff/internal/integrations/kubernetes/internal_k8s_client.go(2 hunks)clients/ui/bff/internal/integrations/kubernetes/k8mocks/internal_k8s_client_mock.go(2 hunks)clients/ui/bff/internal/integrations/kubernetes/k8mocks/token_k8s_client_mock.go(2 hunks)clients/ui/bff/internal/integrations/kubernetes/shared_k8s_client.go(3 hunks)clients/ui/bff/internal/integrations/kubernetes/types.go(1 hunks)clients/ui/bff/internal/integrations/mrserver/http.go(3 hunks)clients/ui/bff/internal/integrations/mrserver/http_test.go(1 hunks)clients/ui/bff/internal/models/model_registry.go(1 hunks)clients/ui/bff/internal/repositories/model_registry.go(3 hunks)clients/ui/frontend/.env(0 hunks)clients/ui/frontend/config/dotenv.js(1 hunks)clients/ui/frontend/config/webpack.common.js(1 hunks)clients/ui/frontend/config/webpack.dev.js(1 hunks)clients/ui/frontend/docs/env-variables.md(0 hunks)clients/ui/frontend/package.json(2 hunks)clients/ui/frontend/src/app/App.tsx(4 hunks)clients/ui/frontend/src/app/api/__tests__/service.spec.ts(1 hunks)clients/ui/frontend/src/app/hooks/useUser.ts(1 hunks)clients/ui/frontend/src/app/pages/modelRegistry/ModelRegistryRoutes.tsx(4 hunks)clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsTabs.tsx(2 hunks)clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx(1 hunks)clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx(1 hunks)clients/ui/frontend/src/app/pages/settings/ModelRegistriesTableRow.tsx(3 hunks)clients/ui/frontend/src/app/utilities/const.ts(1 hunks)clients/ui/frontend/src/bootstrap.tsx(1 hunks)manifests/kustomize/options/ui/overlays/istio/kustomization.yaml(1 hunks)manifests/kustomize/options/ui/overlays/kubeflow/model-registry-ui-deployment.yaml(1 hunks)manifests/kustomize/options/ui/overlays/standalone/auth-proxy-configmap.yaml(1 hunks)manifests/kustomize/options/ui/overlays/standalone/kustomization.yaml(2 hunks)manifests/kustomize/options/ui/overlays/standalone/model-registry-ui-deployment.yaml(1 hunks)manifests/kustomize/options/ui/overlays/standalone/model-registry-ui-service.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- clients/ui/frontend/docs/env-variables.md
- clients/ui/frontend/.env
- clients/python/tests/test_client.py
🧰 Additional context used
🧬 Code Graph Analysis (9)
clients/ui/bff/internal/api/model_registry_settings_rbac_handler_test.go (1)
clients/ui/bff/internal/config/environment.go (3)
EnvConfig(75-104)DeploymentMode(29-29)DeploymentModeStandalone(37-37)
clients/ui/bff/internal/api/model_registry_settings_groups_handler_test.go (1)
clients/ui/bff/internal/config/environment.go (3)
EnvConfig(75-104)DeploymentMode(29-29)DeploymentModeStandalone(37-37)
clients/ui/frontend/config/webpack.common.js (3)
clients/ui/frontend/config/webpack.dev.js (3)
BASE_PATH(26-26)DEPLOYMENT_MODE(25-25)PUBLIC_PATH(18-18)clients/ui/frontend/config/dotenv.js (2)
DEPLOYMENT_MODE(148-148)PUBLIC_PATH(150-150)clients/ui/frontend/src/app/utilities/const.ts (1)
DEPLOYMENT_MODE(22-22)
clients/ui/frontend/config/webpack.dev.js (3)
clients/ui/frontend/config/webpack.common.js (3)
BASE_PATH(21-21)DEPLOYMENT_MODE(20-20)PUBLIC_PATH(12-12)clients/ui/frontend/config/dotenv.js (2)
DEPLOYMENT_MODE(148-148)PUBLIC_PATH(150-150)clients/ui/frontend/src/app/utilities/const.ts (1)
DEPLOYMENT_MODE(22-22)
clients/ui/bff/internal/integrations/mrserver/http.go (1)
clients/ui/bff/internal/api/errors.go (1)
ErrorResponse(17-20)
clients/ui/bff/internal/repositories/model_registry.go (1)
clients/ui/bff/internal/models/model_registry.go (1)
ModelRegistryModel(3-9)
clients/ui/bff/internal/integrations/mrserver/http_test.go (2)
clients/ui/bff/internal/integrations/mrserver/http.go (2)
NewHTTPClient(43-53)HTTPError(34-37)clients/ui/bff/internal/api/errors.go (1)
HTTPError(12-15)
clients/ui/bff/cmd/main.go (1)
clients/ui/bff/internal/config/environment.go (5)
DeploymentMode(29-29)DefaultAuthTokenHeader(21-21)DefaultAuthTokenPrefix(25-25)DeploymentModeStandalone(37-37)DeploymentModeFederated(35-35)
clients/python/tests/test_api.py (1)
clients/python/tests/conftest.py (3)
auth_headers(393-398)cleanup_artifacts(358-371)artifact_resource(374-390)
🪛 Flake8 (7.2.0)
clients/python/tests/test_api_stateful.py
[error] 11-11: at least two spaces before inline comment
(E261)
[error] 11-11: inline comment should start with '# '
(E262)
clients/python/tests/test_api.py
[error] 26-26: expected 2 blank lines, found 0
(E302)
[error] 45-45: expected 2 blank lines, found 1
(E302)
[error] 96-96: too many blank lines (2)
(E303)
clients/python/tests/conftest.py
[error] 343-343: expected 2 blank lines, found 1
(E302)
[error] 344-344: whitespace before ')'
(E202)
[error] 357-357: expected 2 blank lines, found 1
(E302)
[error] 361-361: expected 1 blank line before a nested definition, found 0
(E306)
[error] 373-373: expected 2 blank lines, found 1
(E302)
[error] 392-392: expected 2 blank lines, found 1
(E302)
[error] 400-400: expected 2 blank lines, found 1
(E302)
[error] 418-418: expected 1 blank line before a nested definition, found 0
(E306)
🪛 Pylint (3.3.7)
clients/python/tests/test_api_stateful.py
[refactor] 6-6: Too few public methods (1/2)
(R0903)
🪛 YAMLlint (1.37.1)
manifests/kustomize/options/ui/overlays/standalone/model-registry-ui-deployment.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
manifests/kustomize/options/ui/overlays/standalone/auth-proxy-configmap.yaml
[error] 9-9: trailing spaces
(trailing-spaces)
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
[error] 53-53: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Ruff (0.11.9)
clients/python/tests/conftest.py
409-409: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Test against Py 3.10 and K8s v1.27.11
- GitHub Check: Test against Py 3.9 and K8s v1.27.11
- GitHub Check: Test against Py 3.12 and K8s v1.27.11
- GitHub Check: Test against Py 3.11 and K8s v1.27.11
- GitHub Check: build-and-test-csi-image
- GitHub Check: run-robot-tests
- GitHub Check: build-and-test-image
- GitHub Check: build-image
- GitHub Check: test-and-build
- GitHub Check: check-mysql-schema-structs
- GitHub Check: build
🔇 Additional comments (66)
clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx (1)
16-16: Import path correction looks goodThe path now matches the new
contextfolder layout and prevents broken imports during the build. 👍clients/ui/frontend/src/app/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx (1)
20-20: Consistent context importSame 👍 for aligning with the new directory structure.
clients/ui/frontend/src/app/hooks/useUser.ts (1)
3-3: Path updated correctlyKeeps the hook in sync with the refactor.
clients/python/.gitignore (1)
10-10: Good call adding.hypothesis/Prevents noisy property-based testing artifacts from slipping into git history.
README.md (1)
26-26: LGTM! Documentation link updated correctly.The URL path change from
/apps/to/applications/maintains accurate documentation references and aligns with the upstream repository structure changes.clients/python/tests/constants.py (1)
1-15: LGTM! Well-structured test constants.The constants are appropriately defined for the fuzz testing infrastructure:
ARTIFACT_STATEScovers all expected artifact lifecycle statesARTIFACT_TYPE_PARAMSprovides good test data variety with S3 and documentation URLsDEFAULT_API_TIMEOUTof 5.0 seconds is reasonable for API testingmanifests/kustomize/options/ui/overlays/istio/kustomization.yaml (1)
5-5: Verified: No remaining “integrated” references; all overlays consistently use “kubeflow.”
- Ran a search across manifests and confirmed zero occurrences of “integrated”
- Confirmed “kubeflow” appears in all relevant UI overlay kustomizations
Approving this change.
clients/ui/bff/internal/integrations/kubernetes/types.go (1)
9-9: LGTM! Clean addition of HTTPS support.The
IsHTTPSfield is a well-named boolean addition that integrates cleanly with the existingServiceDetailsstruct to support HTTPS detection for Kubernetes services.clients/ui/bff/internal/models/model_registry.go (1)
8-8: LGTM! Consistent HTTPS support in model layer.The
IsHTTPSfield addition with theisHttpsJSON tag is consistent with the HTTPS awareness being added to the ServiceDetails struct and maintains proper JSON serialization conventions.clients/ui/frontend/package.json (1)
95-95: LGTM: Dependency updates support deployment mode refactoring.The version bumps for
ts-jest(29.2.6 → 29.4.0) andmod-arch-shared(0.1.5 → 0.1.8) align with the broader changes to support the newDeploymentModeenum and kubeflow/federated modes mentioned in the AI summary.Also applies to: 119-119
clients/python/README.md (1)
348-352: LGTM: Clear documentation for new fuzz testing feature.The addition properly documents the new
make test-fuzztarget, following the established documentation pattern and complementing the existing test instructions.clients/ui/frontend/config/webpack.common.js (1)
21-21: LGTM: Deployment mode condition updated correctly.The change from
'integrated'to'kubeflow'correctly aligns with the broader deployment mode refactoring. This is consistent with similar updates inclients/ui/frontend/config/webpack.dev.jsandclients/ui/frontend/config/dotenv.jswhere the same condition is applied.clients/ui/bff/internal/api/model_registry_settings_rbac_handler_test.go (1)
29-29: LGTM: Test configuration updated for new deployment mode enum.The change correctly migrates from the deprecated
StandaloneMode: trueboolean field to the newDeploymentMode: config.DeploymentModeStandaloneenum field. This aligns with theEnvConfigstruct refactoring shown inclients/ui/bff/internal/config/environment.gowhereDeploymentModereplaces the deprecated boolean flags.clients/ui/frontend/config/webpack.dev.js (1)
26-26: LGTM - Consistent deployment mode refactoringThe change from
'integrated'to'kubeflow'aligns with the broader refactoring to use a unifiedDeploymentModeenum. This is consistent with similar changes in other webpack configuration files.clients/python/schemathesis.toml (1)
1-5: LGTM - Well-structured fuzz testing configurationThe Schemathesis configuration is clean and appropriate:
- Using
${API_HOST}environment variable provides deployment flexibility- Disabling shrinking (
no-shrink = true) is a reasonable optimization for CI/CD environments to save time during fuzz testingclients/ui/bff/internal/integrations/kubernetes/k8mocks/internal_k8s_client_mock.go (1)
38-38: LGTM - Consistent mock enhancement for HTTPS detectionThe addition of
IsHTTPS = falseto the mock ServiceDetails is appropriate for local testing scenarios using127.0.0.1:8080. The implementation is consistent across bothGetServiceDetailsandGetServiceDetailsByNamemethods.Also applies to: 52-52
clients/ui/bff/internal/integrations/kubernetes/k8mocks/token_k8s_client_mock.go (1)
43-43: LGTM - Consistent mock implementationThe addition of
IsHTTPS = falsemaintains consistency with theInternalKubernetesClientMockimplementation. Both mock classes now properly initialize the newIsHTTPSfield for local testing scenarios.Also applies to: 57-57
clients/ui/frontend/src/bootstrap.tsx (1)
12-12: LGTM - Consistent removal of deprecated PLATFORM_MODEThe removal of
PLATFORM_MODEfrom the import and the correspondingplatformModeproperty frommodularArchConfigaligns with the broader refactoring to use the unifiedDEPLOYMENT_MODEenum.Note: The AI summary mentions
STYLE_THEMEwas also removed, but it's still present in the import and used on line 26, which appears to be correct based on the actual code.Likely an incorrect or invalid review comment.
clients/ui/bff/internal/api/model_registry_settings_groups_handler_test.go (1)
24-24: LGTM! Test configuration updated to use new deployment mode approach.The change correctly updates the test configuration from the deprecated boolean
StandaloneModeflag to the newDeploymentModeenum, maintaining the same test behavior while aligning with the broader refactoring.manifests/kustomize/options/ui/overlays/kubeflow/model-registry-ui-deployment.yaml (1)
4-4: LGTM! Container argument updated to use explicit deployment mode.The change correctly replaces the boolean
--standalone-mode=falseflag with the explicit--deployment-mode=kubeflowargument, making the deployment mode clear and consistent with the unified deployment mode configuration approach.clients/ui/.env (1)
6-9: LGTM! Environment variables added for new deployment modes and theming.The added environment variables correctly support the new features:
IMG_UI_FEDERATED: Enables federated deployment mode container imageSTYLE_THEME: Introduces UI theming capability with MUI themeDEPLOYMENT_MODE: Sets appropriate default deployment mode for kubeflowclients/ui/frontend/src/app/api/__tests__/service.spec.ts (1)
46-47: LGTM! Mock updated to include new deployment mode values.The mock correctly includes the new
StandaloneandFederateddeployment mode values, ensuring consistency with the actualDeploymentModeenum used in the application.manifests/kustomize/options/ui/overlays/standalone/kustomization.yaml (2)
7-7: LGTM! Auth proxy ConfigMap resource added.The addition of
auth-proxy-configmap.yamlprovides the necessary configuration for the auth proxy in standalone deployment mode.
16-21: LGTM! Service patch added for auth proxy integration.The new patch correctly targets the
model-registry-serviceService to modify its configuration for auth proxy integration in standalone mode.clients/python/noxfile.py (2)
63-63: LGTM: Schemathesis dependency addition is consistent.Adding schemathesis to both existing test sessions ensures consistent testing environment across different test types.
Also applies to: 87-87
104-119: LGTM: Fuzz session implementation follows established patterns.The new fuzz session is well-structured and follows the same pattern as other test sessions. The dependencies are appropriate for fuzz testing, and the pytest flags are correctly configured.
clients/ui/bff/internal/api/middleware.go (1)
115-115: LGTM: Correct integration of IsHTTPS parameter.The addition of
modelRegistry.IsHTTPSparameter to theResolveServerAddresscall is consistent with the broader codebase refactoring to support proper HTTP/HTTPS scheme determination.manifests/kustomize/options/ui/overlays/standalone/model-registry-ui-service.yaml (1)
1-6: LGTM: Port configuration supports new auth-proxy architecture.The port changes correctly configure the service to route external traffic through the auth-proxy container (port 8000) which then forwards to the UI service (port 8080).
clients/ui/frontend/config/dotenv.js (2)
148-148: LGTM: Deployment mode terminology updated consistently.The change from 'integrated' to 'kubeflow' aligns with the broader refactoring effort to standardize deployment mode terminology across the codebase.
154-154: LGTM: HOST logic correctly updated for new deployment mode.The conditional logic properly reflects the new 'kubeflow' deployment mode while maintaining the same behavior (0.0.0.0 for kubeflow, localhost for others).
clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsTabs.tsx (2)
34-35: LGTM: Cleaner config extraction pattern.The refactoring to extract
configand accessdeploymentModefrom it is cleaner and more consistent with the new deployment mode handling approach.
64-64: LGTM: Proper support for federated deployment mode.The updated conditional logic correctly supports both Standalone and Federated deployment modes, removing the legacy platformMode dependency and providing clearer deployment mode handling.
clients/ui/bff/internal/integrations/kubernetes/shared_k8s_client.go (1)
71-89: LGTM! Well-structured HTTPS support implementation.The implementation correctly adds support for both HTTP and HTTPS API ports. The logic properly:
- Checks for both "http-api" and "https-api" port names
- Sets the
isHTTPSflag based on the port name- Updates error messages to reflect both protocols
- Maintains backward compatibility with existing HTTP-only services
clients/python/pyproject.toml (1)
86-89: LGTM! Fuzz testing marker properly configured.The addition of the "fuzz" pytest marker is correctly implemented and aligns with the broader fuzz testing infrastructure being introduced.
clients/ui/frontend/src/app/pages/settings/ModelRegistriesTableRow.tsx (3)
7-7: LGTM! Consistent deployment mode refactoring.The import change from
PlatformModetoDeploymentModealigns with the broader refactoring to unify deployment mode handling across the codebase.
29-31: LGTM! Proper deployment mode implementation.The changes correctly:
- Access
deploymentModefrom theconfigobject- Rename the variable to
isDeploymentKubeflowfor clarity- Maintain the same logical behavior while using the new enum
80-80: LGTM! Variable name updated consistently.The variable rename from
isPlatformKubeflowtoisDeploymentKubeflowis consistent with the broader refactoring and maintains the same functionality.clients/ui/bff/Makefile (2)
6-6: LGTM! Deployment mode variable properly configured.The replacement of
STANDALONE_MODEwithDEPLOYMENT_MODEand the default value ofkubeflowis appropriate for the Open Data Hub context and consistent with the broader refactoring.
62-62: LGTM! Command line flag updated correctly.The update from
--standalone-modeto--deployment-modeis consistent with the new deployment mode approach and properly uses the variable reference..github/workflows/python-tests.yml (1)
231-239: LGTM! Well-structured fuzz testing integration.The new fuzz testing step is properly implemented:
- Correctly runs only on the main branch to avoid excessive resource usage on PRs
- Properly sets up port forwarding for all required services (model-registry, minio, distribution-registry)
- Uses the same setup pattern as the existing e2e tests
- Correctly invokes the fuzz nox session
This integrates well with the broader fuzz testing infrastructure being introduced.
clients/ui/bff/internal/integrations/mrserver/http.go (3)
84-94: Excellent error handling improvement for non-JSON responsesThis enhancement significantly improves robustness by gracefully handling non-JSON error responses (like HTML error pages from proxies or load balancers). The approach of logging a warning with relevant details and creating a generic error response is well-designed.
139-149: Consistent error handling pattern in POST methodThe same robust error handling pattern is correctly applied to the POST method, maintaining consistency across the HTTP client interface.
194-204: Consistent error handling pattern in PATCH methodThe error handling pattern is consistently applied to the PATCH method as well, completing the robustness improvements across all HTTP methods.
clients/ui/bff/internal/api/app.go (1)
164-167: Logic change aligns with deployment mode refactoringThe conditional logic change correctly implements the new deployment mode semantics. Routes are now enabled in all deployment modes except Kubeflow integration mode, which aligns with the broader refactoring from boolean flags to the unified DeploymentMode enum.
clients/ui/frontend/src/app/App.tsx (3)
25-25: Import path update aligns with project structureThe import path change reflects the reorganization of the AppContext into a context directory.
41-43: Deployment mode access updated correctlyThe changes correctly adapt to the new deployment mode structure where
deploymentModeis accessed via theconfigobject, and the boolean logic is properly inverted fromisIntegratedtoisStandalone.
105-117: Conditional rendering updated correctlyThe conditional rendering logic is properly updated to use
isStandaloneinstead of the negatedisIntegrated, maintaining the correct behavior while aligning with the new deployment mode semantics.clients/ui/bff/internal/integrations/mrserver/http_test.go (1)
283-319: Comprehensive test for non-JSON error handlingThis test effectively validates the new error handling functionality for non-JSON responses. It properly sets up an HTML error response scenario and verifies that:
- The client returns an HTTPError
- The status code is correctly preserved
- The error message contains relevant information from the HTML response
- The error code is properly set to the status code string
The test complements the error handling improvements in
http.goand ensures the new functionality works as expected.clients/ui/frontend/src/app/utilities/const.ts (1)
1-24: LGTM! Clean refactor aligning with unified deployment mode.The changes properly remove unused imports and constants while updating defaults to align with the new deployment mode structure. The transition from separate boolean flags to the unified
DeploymentModeenum is implemented correctly.clients/ui/README.md (2)
27-27: LGTM! Clear documentation update.The renaming from "Integrated" to "Kubeflow" better reflects the actual deployment environment and aligns with the unified deployment mode terminology.
52-77: LGTM! Comprehensive documentation for new environment variables.The documentation properly covers the new
IMG_UI_FEDERATEDvariable and updatedDEPLOYMENT_MODEandSTYLE_THEMEoptions with clear descriptions, defaults, and examples.clients/ui/bff/cmd/main.go (2)
34-34: LGTM! Well-implemented unified deployment mode flag.The new
deployment-modeflag properly leverages theconfig.DeploymentModetype and provides a clean interface for the unified deployment mode concept.
44-58: LGTM! Excellent backward compatibility implementation.The code properly handles the transition by:
- Marking old flags as deprecated with clear migration guidance
- Implementing fallback logic that respects existing flag usage
- Maintaining consistency between old and new flag representations
This ensures a smooth migration path for existing deployments.
clients/ui/frontend/src/app/pages/modelRegistry/ModelRegistryRoutes.tsx (2)
20-23: LGTM! Proper update to use unified deployment mode.The changes correctly access the deployment mode through the config object and create clear boolean flags for the specific modes, improving code readability and maintainability.
52-52: LGTM! Consistent deployment mode logic across routes.The conditional rendering properly uses the new boolean flags to enable deployment-related routes only in standalone and federated modes, maintaining the same functionality with clearer logic.
Also applies to: 72-72, 114-114
manifests/kustomize/options/ui/overlays/standalone/model-registry-ui-deployment.yaml (2)
4-4: LGTM! Proper alignment with unified deployment mode.The argument change from
--standalone-mode=trueto--deployment-mode=standalonecorrectly implements the new unified deployment mode flag structure.
6-48: LGTM! Well-configured auth-proxy container.The auth-proxy container is properly configured with:
- Appropriate nginx image version
- Proper security context with non-root user
- Resource limits and requests
- Necessary volume mounts for configuration and temporary storage
clients/python/Makefile (1)
62-63: Good addition of database cleanup stepThe database cleanup ensures a clean state between test runs.
clients/ui/bff/internal/repositories/model_registry.go (1)
62-69: Well-implemented HTTPS supportThe dynamic protocol selection based on the
isHTTPSflag is properly implemented and all callers have been updated correctly.manifests/kustomize/options/ui/overlays/standalone/auth-proxy-configmap.yaml (1)
36-37: Hardcoded credentials in standalone auth proxyThe auth proxy uses a fixed user ID "user@example.com" for all requests. This is appropriate for standalone/development mode but ensure this configuration is not used in production environments.
Also applies to: 43-43
clients/ui/bff/internal/integrations/kubernetes/internal_k8s_client.go (1)
121-251: Excellent concurrency implementation for namespace access checksThe refactored
GetNamespacesmethod efficiently handles namespace access verification with:
- Early exit optimization for cluster admins
- Fixed-size worker pool preventing resource exhaustion
- Proper context cancellation handling throughout
- Comprehensive error logging and metrics
The implementation follows Go concurrency best practices.
clients/ui/bff/internal/config/environment.go (2)
28-73: Well-designed deployment mode enum implementationThe
DeploymentModetype provides:
- Type-safe enum with clear constant definitions
- Proper
flag.Valueinterface implementation with validation- Convenient helper methods for mode checking
- Case-insensitive parsing for better UX
This is a clean refactoring from boolean flags to a unified mode system.
99-103: Good backward compatibility approachMaintaining the deprecated boolean fields ensures existing configurations continue to work while encouraging migration to the new
DeploymentModefield.clients/ui/Makefile (1)
81-95: LGTM! Federated build targets follow established patterns.The new federated Docker build targets are properly implemented with appropriate build arguments for deployment mode and style theme.
clients/python/tests/conftest.py (2)
33-58: Well-implemented test collection logic for fuzz testing.The implementation properly handles mutual exclusion between e2e and fuzz tests, with clear skip reasons.
373-390: Fix formatting and excellent resource management pattern.The context manager implementation is well-designed with proper cleanup.
Apply this diff to fix formatting:
+ @pytest.fixture def artifact_resource():
| APP_ENV=production | ||
| DEPLOYMENT_MODE=integrated | ||
| STYLE_THEME=mui-theme | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removed env-vars may still be referenced
DEPLOYMENT_MODE and STYLE_THEME were dropped. Double-check that no frontend/back-end code or CI script expects them.
🏁 Script executed:
#!/bin/bash
# Detect remaining references to removed variables
rg -n --no-heading -e 'DEPLOYMENT_MODE' -e 'STYLE_THEME' clients/ui | head -20Length of output: 2189
Remove remaining DEPLOYMENT_MODE and STYLE_THEME references
The two vars have been removed from .env.production, but they’re still used throughout the UI repo. Please either restore them with appropriate defaults or strip out all references:
• clients/ui/README.md (lines 64–70, 72–77)
• clients/ui/Makefile (all targets invoking DEPLOYMENT_MODE and STYLE_THEME for start:dev, build, docker build, buildx push, etc.)
• clients/ui/Dockerfile (ARG DEPLOYMENT_MODE, ARG STYLE_THEME)
• clients/ui/frontend/package.json (cypress:server:build script)
• clients/ui/frontend/src/bootstrap.tsx (import and usage of DEPLOYMENT_MODE, STYLE_THEME)
🤖 Prompt for AI Agents
In clients/ui/.env.production at lines 1 to 2, DEPLOYMENT_MODE and STYLE_THEME
variables have been removed but are still referenced in multiple files. To fix
this, either restore these variables in .env.production with appropriate default
values or remove all their references from clients/ui/README.md (lines 64–70,
72–77), clients/ui/Makefile (all targets using these vars),
clients/ui/Dockerfile (ARG declarations), clients/ui/frontend/package.json
(cypress:server:build script), and clients/ui/frontend/src/bootstrap.tsx (import
and usage). Ensure consistency by fully removing or fully restoring these
variables across all mentioned files.
|
|
||
| ARG UI_SOURCE_CODE | ||
| ARG DEPLOYMENT_MODE | ||
| ARG STYLE_THEME |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if STYLE_THEME argument usage is intended.
The STYLE_THEME build argument is declared but not used anywhere in the Dockerfile. If this is intentional preparation for future theming support, consider adding a comment. Otherwise, ensure the argument is properly utilized in the build process.
Let me check if STYLE_THEME is used elsewhere in the build process:
🏁 Script executed:
#!/bin/bash
# Description: Search for STYLE_THEME usage in UI-related files
# Expected: Find usage in Makefiles, environment files, or build scripts
echo "Searching for STYLE_THEME usage..."
rg -A 3 -B 3 "STYLE_THEME" --type-not=mdLength of output: 5064
🏁 Script executed:
#!/bin/bash
# Display the Dockerfile to inspect usage of STYLE_THEME ARG
sed -n '1,200p' clients/ui/DockerfileLength of output: 1452
STYLE_THEME ARG declared but never used in Dockerfile
The ARG STYLE_THEME in clients/ui/Dockerfile (line 15) isn’t referenced in any subsequent instruction, so it has no effect on the front-end build. You should either wire it into the build or document/remove it:
• clients/ui/Dockerfile (after line 15):
- To pass the value into your npm build, add an
ENVor inject it directly:ARG DEPLOYMENT_MODE ARG STYLE_THEME +ENV STYLE_THEME=$STYLE_THEME WORKDIR /usr/src/app +# Now npm sees process.env.STYLE_THEME during build
- Or modify the build step:
# Install deps and build -RUN npm run build:prod +RUN STYLE_THEME=$STYLE_THEME npm run build:prod
• If you’re only reserving this ARG for future theming support, add a clarifying comment above its declaration or remove it until it’s consumed to avoid confusion.
📝 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.
| ARG STYLE_THEME | |
| ARG STYLE_THEME | |
| ENV STYLE_THEME=$STYLE_THEME |
🤖 Prompt for AI Agents
In clients/ui/Dockerfile at line 15, the ARG STYLE_THEME is declared but never
used, making it ineffective. To fix this, either pass STYLE_THEME into the build
process by adding an ENV instruction or injecting it into the npm build command,
or if it is reserved for future use, add a comment above the ARG declaration
explaining its purpose. Alternatively, remove the ARG declaration entirely if it
is not currently needed to avoid confusion.
| black = ">=24.4.2,<26.0.0" | ||
| types-python-dateutil = "^2.9.0.20240906" | ||
| pytest-html = "^4.1.1" | ||
| schemathesis = ">=4.0.3" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the schemathesis version constraint.
The addition of schemathesis as a development dependency looks good. Please verify that version >=4.0.3 is the appropriate constraint and that this version is compatible with the current Python version requirements.
🌐 Web query:
What is the latest version of schemathesis and what are its Python version requirements?
💡 Result:
The latest version of Schemathesis available as of July 2025 is 4.0.0-alpha.8, released on April 5, 2025[2]. This is an alpha (pre-release) version and includes significant internal changes as the project undergoes a major upgrade—production users are advised to stick with the stable 3.x releases until 4.x stabilizes[2].
For Python version requirements:
- Schemathesis 3.39.16 (the latest stable release) is distributed as a Python 3 package, and documentation as well as installation guides commonly show usage with Python 3.10[1][9]. Features like extension hooks are only supported on Python 3.9 and above[7].
- No official documentation or changelog entries list support for Python 2.x; Python 3.8+ is strongly implied as the baseline for stable releases[3][9].
- Certain advanced features, especially extension mechanisms, may require Python 3.9 or newer[7].
Summary Table
| Schemathesis Version | Latest Release Date | Python Version Requirement | Notes |
|---|---|---|---|
| 4.0.0-alpha.8 | 2025-04-05 | Python 3.8+ (assumed), 3.9+ for some features | Alpha/pre-release; not recommended for production[2][7] |
| 3.39.16 (stable) | 2024-xx-xx | Python 3.9+ recommended; 3.8+ likely supported | Most stable and widely used[1][7][9] |
If you need production stability, use Schemathesis 3.39.16 with Python 3.9 or newer. For experimental features and the latest changes, the 4.x alpha is available but may not yet be stable enough for all environments[2][9][7].
If you need precise Python compatibility for Schemathesis 4.x, consult the official documentation or the package metadata after the stable release.
Citations:
- 1: https://schemathesis.readthedocs.io/en/stable/continuous_integration.html
- 2: https://github.com/schemathesis/schemathesis/blob/master/CHANGELOG.md
- 3: https://pypi.org/project/schemathesis/2.3.0/
- 4: https://github.com/orgs/schemathesis/packages/container/package/schemathesis%2Fcache
- 5: https://github.com/schemathesis/schemathesis/blob/master/README.md
- 6: https://sourceforge.net/projects/schemathesis.mirror/
- 7: https://schemathesis.readthedocs.io/en/stable/extending.html
- 8: https://schemathesis.io
- 9: https://pypi.org/project/schemathesis/
- 10: https://testdriven.io/blog/fastapi-hypothesis/
Fix schemathesis version constraint
The current requirement (>=4.0.3) targets a non-existent or prerelease 4.x version. To ensure production stability, pin to the latest stable 3.x release and prevent accidental upgrades to unstable 4.x:
• File: clients/python/pyproject.toml, line 64
- schemathesis = ">=4.0.3"
+ schemathesis = ">=3.39.16,<4.0.0"📝 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.
| schemathesis = ">=4.0.3" | |
| schemathesis = ">=3.39.16,<4.0.0" |
🤖 Prompt for AI Agents
In clients/python/pyproject.toml at line 64, the schemathesis version is set to
">=4.0.3", which targets a non-existent or prerelease 4.x version. Change this
version constraint to pin schemathesis to the latest stable 3.x release by
specifying a version range like ">=3.x, <4.0.0" to avoid accidental upgrades to
unstable 4.x versions and ensure production stability.
| .PHONY: test-fuzz | ||
| test-fuzz: deploy-latest-mr deploy-local-registry deploy-test-minio | ||
| @echo "Starting test-fuzz" | ||
| poetry install --all-extras | ||
| @set -a; . ../../scripts/manifests/minio/.env; set +a; \ | ||
| poetry run pytest --fuzz -v -s --hypothesis-show-statistics | ||
| @rm -f ../../scripts/manifests/minio/.env | ||
| $(MAKE) test-e2e-cleanup | ||
| @exit $$STATUS |
There was a problem hiding this comment.
Fix undefined variable $STATUS
The test-fuzz target uses $STATUS on line 49 without capturing the exit status of the pytest command. This will always exit with status 0.
Apply this fix to capture and use the exit status correctly:
.PHONY: test-fuzz
test-fuzz: deploy-latest-mr deploy-local-registry deploy-test-minio
@echo "Starting test-fuzz"
poetry install --all-extras
- @set -a; . ../../scripts/manifests/minio/.env; set +a; \
- poetry run pytest --fuzz -v -s --hypothesis-show-statistics
- @rm -f ../../scripts/manifests/minio/.env
+ -@set -a; . ../../scripts/manifests/minio/.env; set +a; \
+ poetry run pytest --fuzz -v -s --hypothesis-show-statistics; STATUS=$$?
+ @rm -f ../../scripts/manifests/minio/.env
$(MAKE) test-e2e-cleanup
@exit $$STATUSCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In clients/python/Makefile lines 41 to 49, the variable $STATUS is used on line
49 without being set, causing the script to always exit with status 0. To fix
this, capture the exit status of the pytest command immediately after it runs by
saving it into a variable (e.g., STATUS=$$?), then use this variable in the exit
command. Modify the script to store the exit code of the pytest command before
cleanup and exit.
| assert response_json.get("artifactType") == payload["artifactType"], "Response artifactType should match payload" | ||
|
|
||
|
|
||
| @pytest.mark.fuzz |
There was a problem hiding this comment.
Fix formatting issues.
Apply this diff to fix the formatting:
+
@pytest.mark.fuzzAnd remove the extra blank line:
create_payload["modelFormatVersion"] = "1.0"
-
with artifact_resource(auth_headers, create_payload) as artifact_id:Also applies to: 95-96
🤖 Prompt for AI Agents
In clients/python/tests/test_api.py at lines 76 and 95-96, fix the formatting
issues by removing any extra blank lines around the @pytest.mark.fuzz decorator
and ensuring consistent spacing. Adjust the lines so that the decorator is
directly above the test function definition without additional blank lines
before or after it.
| .PHONY: frontend-build | ||
| frontend-build: | ||
| cd frontend && npm run build:prod | ||
| cd frontend && DEPLOYMENT_MODE=kubeflow && npm run build:prod |
There was a problem hiding this comment.
Fix environment variable syntax.
The current syntax won't properly set the environment variable.
Apply this diff:
- cd frontend && DEPLOYMENT_MODE=kubeflow && npm run build:prod
+ cd frontend && DEPLOYMENT_MODE=kubeflow npm run build:prod📝 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.
| cd frontend && DEPLOYMENT_MODE=kubeflow && npm run build:prod | |
| cd frontend && DEPLOYMENT_MODE=kubeflow npm run build:prod |
🤖 Prompt for AI Agents
In clients/ui/Makefile at line 124, the environment variable DEPLOYMENT_MODE is
not set correctly for the npm run command. Fix this by using the correct syntax
to export the environment variable inline before the command, such as prefixing
the command with DEPLOYMENT_MODE=kubeflow, ensuring it applies only to the npm
run build:prod execution within the frontend directory.
| @pytest.fixture | ||
| def auth_headers(setup_env_user_token): | ||
| """Provides authorization headers for API requests.""" | ||
| return { | ||
| "Content-Type": "application/json", | ||
| "Authorization": f"Bearer {setup_env_user_token}" | ||
| } |
There was a problem hiding this comment.
Fix token usage bug and formatting.
The setup_env_user_token fixture returns a file path, not the token content. You need to read the token from the file.
Apply this diff to fix the bug and formatting:
+
@pytest.fixture
def auth_headers(setup_env_user_token):
"""Provides authorization headers for API requests."""
+ with open(setup_env_user_token, 'r') as f:
+ token = f.read().strip()
return {
"Content-Type": "application/json",
- "Authorization": f"Bearer {setup_env_user_token}"
+ "Authorization": f"Bearer {token}"
}📝 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.
| @pytest.fixture | |
| def auth_headers(setup_env_user_token): | |
| """Provides authorization headers for API requests.""" | |
| return { | |
| "Content-Type": "application/json", | |
| "Authorization": f"Bearer {setup_env_user_token}" | |
| } | |
| @pytest.fixture | |
| def auth_headers(setup_env_user_token): | |
| """Provides authorization headers for API requests.""" | |
| with open(setup_env_user_token, 'r') as f: | |
| token = f.read().strip() | |
| return { | |
| "Content-Type": "application/json", | |
| "Authorization": f"Bearer {token}" | |
| } |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 392-392: expected 2 blank lines, found 1
(E302)
🤖 Prompt for AI Agents
In clients/python/tests/conftest.py around lines 392 to 398, the auth_headers
fixture incorrectly uses setup_env_user_token as the token itself, but it
actually returns a file path. To fix this, open and read the token string from
the file path returned by setup_env_user_token, then use the token content in
the Authorization header. Also, ensure the returned dictionary is properly
formatted with correct indentation and string usage.
| @pytest.fixture | ||
| def cleanup_artifacts(request: pytest.FixtureRequest, auth_headers: dict): | ||
| """Cleanup artifacts created during the test.""" | ||
| created_ids = [] | ||
| def register(artifact_id): | ||
| created_ids.append(artifact_id) | ||
|
|
||
| yield register | ||
|
|
||
| for artifact_id in created_ids: | ||
| del_url = f"{REGISTRY_URL}/api/model_registry/v1alpha3/artifacts/{artifact_id}" | ||
| try: | ||
| requests.delete(del_url, headers=auth_headers, timeout=DEFAULT_API_TIMEOUT) | ||
| except Exception as e: | ||
| print(f"Failed to delete artifact {artifact_id}: {e}") | ||
|
|
There was a problem hiding this comment.
Fix formatting issues.
Apply this diff to fix formatting:
+
@pytest.fixture
def cleanup_artifacts(request: pytest.FixtureRequest, auth_headers: dict):
"""Cleanup artifacts created during the test."""
created_ids = []
+
def register(artifact_id):📝 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.
| @pytest.fixture | |
| def cleanup_artifacts(request: pytest.FixtureRequest, auth_headers: dict): | |
| """Cleanup artifacts created during the test.""" | |
| created_ids = [] | |
| def register(artifact_id): | |
| created_ids.append(artifact_id) | |
| yield register | |
| for artifact_id in created_ids: | |
| del_url = f"{REGISTRY_URL}/api/model_registry/v1alpha3/artifacts/{artifact_id}" | |
| try: | |
| requests.delete(del_url, headers=auth_headers, timeout=DEFAULT_API_TIMEOUT) | |
| except Exception as e: | |
| print(f"Failed to delete artifact {artifact_id}: {e}") | |
| @pytest.fixture | |
| def cleanup_artifacts(request: pytest.FixtureRequest, auth_headers: dict): | |
| """Cleanup artifacts created during the test.""" | |
| created_ids = [] | |
| def register(artifact_id): | |
| created_ids.append(artifact_id) | |
| yield register | |
| for artifact_id in created_ids: | |
| del_url = f"{REGISTRY_URL}/api/model_registry/v1alpha3/artifacts/{artifact_id}" | |
| try: | |
| requests.delete(del_url, headers=auth_headers, timeout=DEFAULT_API_TIMEOUT) | |
| except Exception as e: | |
| print(f"Failed to delete artifact {artifact_id}: {e}") |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 357-357: expected 2 blank lines, found 1
(E302)
[error] 361-361: expected 1 blank line before a nested definition, found 0
(E306)
🤖 Prompt for AI Agents
In clients/python/tests/conftest.py around lines 357 to 372, the code has
inconsistent or incorrect formatting. Fix the formatting by ensuring proper
indentation, spacing, and line breaks according to Python style guidelines,
making the code more readable and maintainable.
| @pytest.fixture | ||
| def state_machine(generated_schema: BaseOpenAPISchema, auth_headers: str) -> APIStateMachine: | ||
| BaseAPIWorkflow = generated_schema.as_state_machine() | ||
|
|
||
| class APIWorkflow(BaseAPIWorkflow): # type: ignore | ||
| headers: dict[str, str] | ||
|
|
||
| def setup(self) -> None: | ||
| print("Cleaning up database") | ||
| subprocess.run( | ||
| ["../../scripts/cleanup.sh"], | ||
| capture_output=True, | ||
| check=True | ||
| ) | ||
| self.headers = auth_headers | ||
|
|
||
| def before_call(self, case: Case) -> None: | ||
| print(f"Checking: {case.method} {case.path}") | ||
| def get_call_kwargs(self, case: Case) -> dict[str, Any]: | ||
| return {"verify": False, "headers": self.headers} | ||
|
|
||
| def after_call(self, response: Response, case: Case) -> None: | ||
| print(f"{case.method} {case.path} -> {response.status_code},") | ||
| return APIWorkflow |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve script path handling and fix formatting.
The hardcoded relative path to the cleanup script is fragile. Consider using the root fixture to construct the path.
Apply this diff to improve path handling and fix formatting:
+
@pytest.fixture
-def state_machine(generated_schema: BaseOpenAPISchema, auth_headers: str) -> APIStateMachine:
+def state_machine(generated_schema: BaseOpenAPISchema, auth_headers: dict, root: Path) -> APIStateMachine:
BaseAPIWorkflow = generated_schema.as_state_machine()
class APIWorkflow(BaseAPIWorkflow): # type: ignore
headers: dict[str, str]
def setup(self) -> None:
print("Cleaning up database")
subprocess.run(
- ["../../scripts/cleanup.sh"],
+ [str(root / "scripts" / "cleanup.sh")],
capture_output=True,
check=True
)
self.headers = auth_headers
def before_call(self, case: Case) -> None:
print(f"Checking: {case.method} {case.path}")
+
def get_call_kwargs(self, case: Case) -> dict[str, Any]:
return {"verify": False, "headers": self.headers}Also note: the type hint for auth_headers parameter was incorrect (str instead of dict).
📝 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.
| @pytest.fixture | |
| def state_machine(generated_schema: BaseOpenAPISchema, auth_headers: str) -> APIStateMachine: | |
| BaseAPIWorkflow = generated_schema.as_state_machine() | |
| class APIWorkflow(BaseAPIWorkflow): # type: ignore | |
| headers: dict[str, str] | |
| def setup(self) -> None: | |
| print("Cleaning up database") | |
| subprocess.run( | |
| ["../../scripts/cleanup.sh"], | |
| capture_output=True, | |
| check=True | |
| ) | |
| self.headers = auth_headers | |
| def before_call(self, case: Case) -> None: | |
| print(f"Checking: {case.method} {case.path}") | |
| def get_call_kwargs(self, case: Case) -> dict[str, Any]: | |
| return {"verify": False, "headers": self.headers} | |
| def after_call(self, response: Response, case: Case) -> None: | |
| print(f"{case.method} {case.path} -> {response.status_code},") | |
| return APIWorkflow | |
| @pytest.fixture | |
| def state_machine(generated_schema: BaseOpenAPISchema, auth_headers: dict, root: Path) -> APIStateMachine: | |
| BaseAPIWorkflow = generated_schema.as_state_machine() | |
| class APIWorkflow(BaseAPIWorkflow): # type: ignore | |
| headers: dict[str, str] | |
| def setup(self) -> None: | |
| print("Cleaning up database") | |
| subprocess.run( | |
| [str(root / "scripts" / "cleanup.sh")], | |
| capture_output=True, | |
| check=True | |
| ) | |
| self.headers = auth_headers | |
| def before_call(self, case: Case) -> None: | |
| print(f"Checking: {case.method} {case.path}") | |
| def get_call_kwargs(self, case: Case) -> dict[str, Any]: | |
| return {"verify": False, "headers": self.headers} | |
| def after_call(self, response: Response, case: Case) -> None: | |
| print(f"{case.method} {case.path} -> {response.status_code},") | |
| return APIWorkflow |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 400-400: expected 2 blank lines, found 1
(E302)
[error] 418-418: expected 1 blank line before a nested definition, found 0
(E306)
🪛 Ruff (0.11.9)
409-409: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
In clients/python/tests/conftest.py around lines 400 to 423, the cleanup script
path is hardcoded as a relative path, which is fragile and can break depending
on the working directory. Use the provided root fixture to construct an absolute
path to the cleanup script instead. Also, fix the indentation and formatting
issues in the method definitions, and correct the type hint of the auth_headers
parameter from str to dict[str, str]. Update the subprocess.run call to use the
constructed path and ensure consistent formatting throughout the class.
| @pytest.fixture(scope="session") | ||
| def generated_schema(pytestconfig: pytest.Config ) -> BaseOpenAPISchema: | ||
| """Generate the schema for the API""" | ||
|
|
||
| os.environ["API_HOST"] = REGISTRY_URL | ||
| config = schemathesis.config.SchemathesisConfig.from_path(f"{pytestconfig.rootpath}/schemathesis.toml") | ||
| local_schema_path = f"{pytestconfig.rootpath}/../../api/openapi/model-registry.yaml" | ||
| schema = schemathesis.openapi.from_path( | ||
| path=local_schema_path, | ||
| config=config, | ||
| ) | ||
| schema.config.output.sanitization.update(enabled=False) | ||
| return schema |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Fix formatting and consider sanitization implications.
Apply this diff to fix formatting:
+
@pytest.fixture(scope="session")
-def generated_schema(pytestconfig: pytest.Config ) -> BaseOpenAPISchema:
+def generated_schema(pytestconfig: pytest.Config) -> BaseOpenAPISchema:Note: Output sanitization is disabled on line 354. Ensure this doesn't expose sensitive data in test logs.
Fix formatting and reconsider output sanitization
• clients/python/tests/conftest.py: add a blank line before the @pytest.fixture decorator (E302).
• clients/python/tests/conftest.py: remove the extra space before the closing parenthesis in the generated_schema signature (E201).
diff --git a/clients/python/tests/conftest.py b/clients/python/tests/conftest.py
@@ -341,6 +341,7 @@
+
@pytest.fixture(scope="session")
-def generated_schema(pytestconfig: pytest.Config ) -> BaseOpenAPISchema:
+def generated_schema(pytestconfig: pytest.Config) -> BaseOpenAPISchema:
"""Generate the schema for the API"""Note: Disabling sanitization via schema.config.output.sanitization.update(enabled=False) may expose sensitive data in test logs—ensure this change is intentional or re-enable sanitization for non-debug scenarios.
📝 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.
| @pytest.fixture(scope="session") | |
| def generated_schema(pytestconfig: pytest.Config ) -> BaseOpenAPISchema: | |
| """Generate the schema for the API""" | |
| os.environ["API_HOST"] = REGISTRY_URL | |
| config = schemathesis.config.SchemathesisConfig.from_path(f"{pytestconfig.rootpath}/schemathesis.toml") | |
| local_schema_path = f"{pytestconfig.rootpath}/../../api/openapi/model-registry.yaml" | |
| schema = schemathesis.openapi.from_path( | |
| path=local_schema_path, | |
| config=config, | |
| ) | |
| schema.config.output.sanitization.update(enabled=False) | |
| return schema | |
| @pytest.fixture(scope="session") | |
| def generated_schema(pytestconfig: pytest.Config) -> BaseOpenAPISchema: | |
| """Generate the schema for the API""" | |
| os.environ["API_HOST"] = REGISTRY_URL | |
| config = schemathesis.config.SchemathesisConfig.from_path(f"{pytestconfig.rootpath}/schemathesis.toml") | |
| local_schema_path = f"{pytestconfig.rootpath}/../../api/openapi/model-registry.yaml" | |
| schema = schemathesis.openapi.from_path( | |
| path=local_schema_path, | |
| config=config, | |
| ) | |
| schema.config.output.sanitization.update(enabled=False) | |
| return schema |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 343-343: expected 2 blank lines, found 1
(E302)
[error] 344-344: whitespace before ')'
(E202)
🤖 Prompt for AI Agents
In clients/python/tests/conftest.py around lines 343 to 355, fix the formatting
by adding a blank line before the @pytest.fixture decorator and remove the extra
space before the closing parenthesis in the generated_schema function signature.
Also, review the line disabling output sanitization
(schema.config.output.sanitization.update(enabled=False)) and either remove or
conditionally enable it to avoid exposing sensitive data in test logs unless
explicitly intended for debugging.
openshift-merge-bot
Bot
merged commit b2daef0
into
opendatahub-io:main
… to 0.2.4-alpha in /clients/ui/bff (opendatahub-io#244) Bumps [github.com/kubeflow/model-registry](https://github.com/kubeflow/model-registry) from 0.2.3-alpha to 0.2.4-alpha. - [Release notes](https://github.com/kubeflow/model-registry/releases) - [Commits](kubeflow/hub@v0.2.3-alpha...v0.2.4-alpha) --- updated-dependencies: - dependency-name: github.com/kubeflow/model-registry dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
5 participants
by adding the fuzz test step in job here in m/s
labels are applied
Summary by CodeRabbit
New Features
Improvements
DEPLOYMENT_MODE.Bug Fixes
Chores
.gitignoreand documentation for clarity.Tests