Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR to fix Neptune connections with SIG4 different service names #218

Closed
wants to merge 11 commits into from

Conversation

cubeddu
Copy link
Contributor

@cubeddu cubeddu commented Nov 10, 2023

Issue #217

Description

This pull request introduces a new feature that enhances the flexibility of AWS Neptune IAM connections in the Graph Explorer. The main enhancement is the ability to switch the service type between "neptune-db" and "neptune-graph" for connections created through the Graph Explorer UI or set as the default connection.

Changes Made

Removed RequestSig.js as is no longer being used.

UI

image

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (39eed2c) 10.01% compared to head (d3b203f) 10.02%.

Files Patch % Lines
.../src/modules/CreateConnection/CreateConnection.tsx 0.00% 34 Missing ⚠️
.../src/modules/ConnectionDetail/ConnectionDetail.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #218   +/-   ##
=======================================
  Coverage   10.01%   10.02%           
=======================================
  Files         407      407           
  Lines       30091    30113   +22     
  Branches      655      655           
=======================================
+ Hits         3014     3018    +4     
- Misses      26742    26760   +18     
  Partials      335      335           
Flag Coverage Δ
unittests 10.02% <10.25%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@michaelnchin michaelnchin left a comment

Choose a reason for hiding this comment

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

Thanks @cubeddu and @charlesivie !

I tested the new service type parameter out via a SageMaker Notebook stack, and openCypher connections were working great against neptune-graph.

Couple of issues/requests I'd like to see addressed:

  1. When initializing Explorer with SERVICE_TYPE env variable set to neptune-graph, the primary default connection displayed on open should be changed from Gremlin -> openCypher. The primary connection should remain as Gremlin as the default and for neptune-db.

  2. When service name is initially set as neptune-graph via the SERVICE_TYPE env variable, Explorer will use correctly use the indicated service for requests. However, this is not visually reflected in the Connections UI, where all default profiles will continue to display the default "Neptune Graph" in the "Service Type" field.

  3. Updating the "Service Type" in a Connections UI profile isn't functioning as intended after using the SERVICE_TYPE env variable. i.e. if we initially set the service to neptune-graph, all requests from both default and manually created connection profiles will continue to be signed with neptune-graph, regardless of the "Service Type" selected (see second point).

README.md Outdated Show resolved Hide resolved
@cubeddu
Copy link
Contributor Author

cubeddu commented Nov 16, 2023

@michaelnchin As part of the updates, we made sure to allow the service type to flow all the way to the proxy server via headers, so now you will be able to update the service type per new connection or update the current one.

these changes should handle all 3 cases.

@cubeddu cubeddu closed this Nov 20, 2023
@cubeddu cubeddu reopened this Nov 30, 2023
@vkagamlyk
Copy link
Contributor

superseded by #241

@cubeddu cubeddu closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🗑 Discarded
Development

Successfully merging this pull request may close these issues.

3 participants