-
Notifications
You must be signed in to change notification settings - Fork 48
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
Adds Default Connection Support #108
Adds Default Connection Support #108
Conversation
Moving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in comments, I think we should explore alternative ways to achieve the desired outcome here. Running a build step each time the container launches (that takes 30 seconds or more on my test system) is not ideal and will lead to user confusion and other issues around when the container is actually available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested the PR for HTTP and HTTPS locally following the instructions from @jackson-millard above. I tested it with both a .json configuration file and runtime environment variables. As far as I understand all the comments above are addressed with this commit. If there are no further concerns in my opinion this PR is ready for approval. Anyone else has tested and can provide feedback? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jackson-millard ! I've tested the changes in this PR with the SageMaker development branch, and everything works as described when Explorer is started with default connection arguments via a Lifecycle configuration.
A few feature changes to request:
- In cases where the optional "GRAPH_TYPE" field/argument is not specified, we currently default to creating one profile on Gremlin. Can the default behavior be changed so that Explorer instead creates a separate connection profile for every available language? The rest of the connection fields should be identical in each profile.
- Somewhat related to point 1 - for default connections, the "GRAPH_TYPE" and "PUBLIC_OR_PROXY_ENDPOINT" fields are grayed out and read-only. If a user made a mistake in initial setup, or would otherwise like to update these fields, they would be required to create a separate connection profile. Can this be changed so that all default connection fields are modifiable while Explorer is running?
… PROXY_SERVER_HTTPS_CONNECTION
… file is not present
@michaelnchin I'm glad you brought up these issues. I've opened a PR against the current PR to address point 2, PR However, I suggest we discuss point 1 further before proceeding. I've raised a concern about treating default connections as fileBase, which could prevent users from deleting them later. To avoid this, we might need to consider an alternative approach and by fixing this, point 2 will be also automatically resolved. I propose merging the current PR as a starting point, and then we can create a new ticket to explore improvements for handling default connections. This way, we can iterate and address the issue more thoroughly. Your thoughts are appreciated. Thanks! |
…guration Agutierrez/add default configuration
Codecov Report
@@ Coverage Diff @@
## main #108 +/- ##
========================================
- Coverage 9.35% 9.35% -0.01%
========================================
Files 463 463
Lines 30743 30765 +22
Branches 235 235
========================================
Hits 2877 2877
- Misses 27860 27882 +22
Partials 6 6
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for all your work on this @jackson-millard and @agutierrezgit !
…DefaultConfiguration
Description:
Modifies the docker build and run process by pushing the
pnpm build
command into thedocker-entrypoint.sh
file so that thedocker run
command can take environment variables and add them to the.env
file before the build is created. This allows for a default connection to be provided. Currently supports a json calledconfig.json
provided via a volume mount or as individual variables provided via--env
. SeeREADME.md
for how to execute either option. From a process standpoint, the variables passed in either manner get processed and appended to the .env file in process-environment.sh so the build can use them.Testing:
This was tested via local builds by providing the json solution, the --env solution, and providing both, in which case the json takes precedence. To test, make sure that your browser is not caching connections from one build to another. Opening an incognito window during testing makes for the easiest way to circumvent this.