Skip to content

feat: extract shared infrastructure into internal/platform layer#2644

Open
Al-Pragliola wants to merge 2 commits intokubeflow:mainfrom
Al-Pragliola:platform/2-mr-refactor-v2
Open

feat: extract shared infrastructure into internal/platform layer#2644
Al-Pragliola wants to merge 2 commits intokubeflow:mainfrom
Al-Pragliola:platform/2-mr-refactor-v2

Conversation

@Al-Pragliola
Copy link
Copy Markdown
Contributor

Description

Introduces internal/platform/, a new package tree that separates reusable infrastructure from model-registry-specific internals, then migrates the model registry to use it.

Problem

The catalog (and any future registry) must import packages from the model registry's internal/ tree — internal/db/, internal/datastore/, internal/apiutils/, etc. These packages mix generic database infrastructure with model-registry-specific logic. This makes the model registry the platform itself rather than a consumer of it, and makes it impossible to add a new registry without depending on model-specific code.

What this does

  1. Creates internal/platform/ with all shared infrastructure: database abstractions (entity model, filtering, query building, repository, schema, scopes, migrations), datastore connectors, error types, TLS config, proxy/health infrastructure, server middleware, and API utilities.
  2. Migrates the model registry to import from internal/platform/ — converts shared packages (db/models/, db/filter/, db/service/, db/types/) to thin re-export shims, updates all internal consumers (core/, converter/, mapper/, proxy/, server/, etc.), and deletes the now-redundant originals.
  3. Migrates the catalog — all catalog files switch from internal/db/, internal/datastore/, and pkg/api imports to their internal/platform/ equivalents. The catalog no longer depends on any model-registry-specific internal package.
  4. Updates tooling — gorm-gen, Makefile, and scripts/remove_gorm_defaults.sh updated to reference the new platform migration and schema paths.

Design constraint

The platform layer has zero imports from model-specific packages. No file in internal/platform/ references internal/core/, internal/db/models/, internal/converter/, internal/server/openapi/, or internal/defaults/.

How Has This Been Tested?

  • go build ./... passes from root and catalog
  • go test ./... -short passes
  • go vet ./... clean (pre-existing participle parser tag warnings only)
  • No file in internal/platform/ imports from model-specific packages
  • Existing functionality unchanged — pure structural refactor

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages
  • Automated tests are provided as part of the PR for major new functionalities; testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.
  • For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
@google-oss-prow
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from al-pragliola. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
@Al-Pragliola
Copy link
Copy Markdown
Contributor Author

Al-Pragliola commented Apr 27, 2026

fuzz tests result https://github.com/kubeflow/hub/actions/runs/25005001401

Copy link
Copy Markdown
Member

@pboyd pboyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general.

Is it possible to clean up comments in another PR? It's hard to follow those changes across renames and they seem outside the focus of this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you restore the doc comments in this file? Some of them may not add much, but some of them do.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments too. It's helpful to know what it's mapping.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And these comments,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the doc comments in this file seem to be restating the function name, but they weren't hurting anything either. Can we restore them?

Comment on lines -15 to -16
// validateStringParameter validates string parameters for unsafe characters
// Null bytes are not allowed in string parameters as they can cause database errors
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems useful because it's otherwise unclear what validation is being done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants