Skip to content
This repository was archived by the owner on Sep 11, 2025. It is now read-only.

Conversation

@mattjohnsonpint
Copy link
Contributor

Description

  • Update to latest DGo client and refactor
  • Support the new connection string format (specified as "connString" in the manifest, similar to other DBs)
  • Misc cleanup

Checklist

All PRs should check the following boxes:

  • I have given this PR a title using the
    Conventional Commits syntax, leading with fix:,
    feat:, chore:, ci:, etc.
    • The title should also be used for the commit message when the PR is squashed and merged.
  • I have formatted and linted my code with Trunk, per the instructions in
    the contributing guide.

If the PR includes a code change, then also check the following boxes. (If not, then delete the
next section.)

  • I have added an entry to the CHANGELOG.md file.
    • Add to the "UNRELEASED" section at the top of the file, creating one if it doesn't yet exist.
    • Be sure to include the link to this PR, and please sort the section numerically by PR number.
  • I have manually tested the new or modified code, and it appears to behave correctly.
  • I have added or updated unit tests where appropriate, if applicable.

@mattjohnsonpint mattjohnsonpint requested review from a team and Copilot March 28, 2025 15:34
@linear
Copy link

linear bot commented Mar 28, 2025

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Dgraph client integration to support a new connection string format by refactoring how connections are created and managed. Key changes include:

  • Replacing the legacy gRPC connection approach with connection string and grpc target based methods.
  • Removing the old auth credentials handling and associated imports.
  • Updating the manifest structure and tests to support the new connection string field.

Reviewed Changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
runtime/dgraphclient/registry.go Refactored connection creation and shutdown logic; added new branches for connection string handling.
runtime/dgraphclient/dgraph.go Removed grpc.ClientConn field and introduced a constructor for the new connector.
lib/manifest/test/manifest_test.go Added tests for the new connection string usage in Dgraph connections.
lib/manifest/dgraph.go Updated DgraphConnectionInfo to include the new connString field.
CHANGELOG.md Changelog updated to include the new Dgraph connection string feature.
Files not reviewed (4)
  • lib/manifest/modus_schema.json: Language not supported
  • lib/manifest/test/valid_modus.json: Language not supported
  • runtime/go.mod: Language not supported
  • sdk/go/go.mod: Language not supported
Comments suppressed due to low confidence (2)

runtime/dgraphclient/registry.go:42

  • Verify that the new dgraph client's Close() method properly releases all underlying resources, as it replaces the previous grpc.ClientConn.Close().
connector.dgClient.Close()

runtime/dgraphclient/registry.go:74

  • [nitpick] Consider rephrasing the error message for when both connString and grpcTarget (or key) are provided to give clearer guidance on which option to use.
if connection.GrpcTarget != "" {

@mattjohnsonpint mattjohnsonpint merged commit 12935cb into main Mar 28, 2025
71 checks passed
@mattjohnsonpint mattjohnsonpint deleted the mjp/hyp-3159-support-new-dgraph-connection-string-syntax-in-modus branch March 28, 2025 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants