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

Adds Default Connection Support #108

Merged
merged 25 commits into from
Aug 15, 2023

Conversation

jackson-millard
Copy link
Contributor

Description:
Modifies the docker build and run process by pushing the pnpm build command into the docker-entrypoint.sh file so that the docker 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 called config.json provided via a volume mount or as individual variables provided via --env. See README.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.

@krlawrence
Copy link
Contributor

krlawrence commented May 24, 2023

Moving pnpm build inside the container this way is problematic. That build process takes around 30 seconds. During that time the container will appear to the user to be up and running but in reality it is still building "behind the scenes". Unless they are tail-ing the Docker logs with something like docker logs -f explorer they will have no idea when it is ready. This also means that each time the container launches it will re-do these build steps. I would like to explore different ways of solving this problem.

@krlawrence krlawrence self-requested a review May 25, 2023 13:56
Copy link
Contributor

@krlawrence krlawrence left a 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.

Copy link
Contributor

@agutierrezgit agutierrezgit left a 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

@joywa joywa requested a review from michaelnchin July 12, 2023 16:49
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 @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:

  1. 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.
  2. 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?
Screen Shot 2023-07-17 at 6 06 33 PM

@agutierrezgit
Copy link
Contributor

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:

  1. 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.
  2. 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?
Screen Shot 2023-07-17 at 6 06 33 PM

@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!

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #108 (c392caf) into main (9c02e2d) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

❗ Current head c392caf differs from pull request most recent head 794352f. Consider uploading reports for the commit 794352f to get more accurate results

@@           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              
Files Changed Coverage Δ
...ckages/graph-explorer/src/core/AppStatusLoader.tsx 0.00% <0.00%> (ø)
.../src/modules/ConnectionDetail/ConnectionDetail.tsx 0.00% <0.00%> (ø)
.../src/modules/CreateConnection/CreateConnection.tsx 0.00% <0.00%> (ø)

@michaelnchin michaelnchin self-requested a review August 5, 2023 02:21
michaelnchin
michaelnchin previously approved these changes Aug 5, 2023
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.

LGTM. Thanks for all your work on this @jackson-millard and @agutierrezgit !

@michaelnchin michaelnchin self-requested a review August 9, 2023 22:38
michaelnchin
michaelnchin previously approved these changes Aug 9, 2023
@michaelnchin michaelnchin merged commit f54fba2 into aws:main Aug 15, 2023
1 check passed
@michaelnchin michaelnchin linked an issue Aug 16, 2023 that may be closed by this pull request
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: ✅ Done
4 participants