Features and code reorganization to support the KBase Lakehouse - #243
Open
jeff-cohere wants to merge 74 commits into
Open
jeff-cohere wants to merge 74 commits into
jeff-cohere wants to merge 74 commits into
Conversation
… Globus share.
Specifically:
* I've added the ability to directly upload files to a Globus share via HTTPS.
* The logic governing Globus access keys has been simplified.
* The Root() method for the Endpoint interface has been broken into:
* a BasePath() method that returns the absolute path on the filesystem
below which files are not visible to a Globus share
* a DataPath() method that returns the path on the filesystem (relative to
BasePath()) where files of interest are located
Additionally, there are various small fixes and cleanups.
…user credentials. Work in progress.
Contributor
|
Collaborator
Author
|
The minio/minio Docker image seems unavailable as of a few minutes ago. Routine botstorm or discontinued? I suppose we'll find out soon. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cross-provider transfers currently contain blocking path, credential-handling, HTTP, concurrency, and secret-persistence defects.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Lakehouse interoperability across local, Globus, and S3 endpoints.
Changes:
- Extends endpoint identity, path, and connection APIs.
- Adds Globus HTTPS and S3 credential integration via MMS.
- Introduces a KBase Lakehouse database stub and configuration updates.
File summaries
| File | Description |
|---|---|
transfers/transfers.go |
Updates provider and database registration. |
transfers/transfers_test.go |
Migrates endpoint path fixtures. |
transfers/store.go |
Simplifies transfer creation messages. |
transfers/mover.go |
Adds cross-provider connection setup. |
services/version.go |
Bumps version to 0.15.0. |
services/prototype.go |
Clarifies KBase authentication fallback. |
integration/irods/fixtures/test-config.yaml |
Migrates local path configuration. |
endpoints/s3/endpoint.go |
Implements expanded endpoint interface. |
endpoints/s3/endpoint_test.go |
Updates S3 path assertions. |
endpoints/local/endpoint.go |
Adds split paths and Globus HTTPS uploads. |
endpoints/local/endpoint_test.go |
Updates local endpoint fixtures. |
endpoints/globus/globus.go |
Adds dedicated Globus API clients. |
endpoints/globus/endpoint.go |
Refactors Globus endpoint integration. |
endpoints/endpoints.go |
Expands the endpoint contract. |
dtstest/dtstest.go |
Updates endpoint test doubles. |
deployment/dts.yaml |
Revises deployment endpoint configuration. |
databases/nmdc/database_test.go |
Migrates Globus path fixtures. |
databases/kbase_lakehouse/database.go |
Adds the Lakehouse database stub. |
auth/kbase_mms.go |
Adds MMS credential retrieval. |
auth/kbase_auth_server.go |
Associates users with connection credentials. |
auth/authenticator.go |
Initializes credential maps. |
auth/auth.go |
Extends users and credentials for connections. |
Review details
Suppressed comments (2)
endpoints/globus/globus.go:712
- As in
get,filepath.Joincorrupts an HTTPS URL (https://…becomeshttps:/…). This makes every Manager API POST, including user-credential registration, fail before reaching Globus.
resourcePath := filepath.Join(c.Url, resource)
endpoints/globus/endpoint.go:271
- This upload path ignores
DataPath, despite the endpoint contract defining it as the directory containing endpoint data. A configured destination such asglobus-kbase(data_path: jeff_cohere) receives local uploads under the base path instead. Apply the destination data path exactly once and reconcile the custom-destination path construction, which currently embeds that path inDestinationPath.
absPath := filepath.Join(ep.Paths.Base, resource)
- Files reviewed: 22/22 changed files
- Comments generated: 16
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jeff-cohere
force-pushed
the
kbase-lakehouse
branch
from
September 21, 2026 23:40
ccb92f4 to
7bc0727
Compare
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR brings in some interoperability features needed to support a deployment of the DTS to the KBase Lakehouse environment:
kbase_lakehousedatabase, different from the existingkbasedatabase, which supports the legacy Narrative environment. This still needs work.Because we're now using several different Globus APIs, I've also reorganized the Globus logic into its own source file. This makes the Globus endpoint logic more transparent.