Feat/persistent-docker-network #734

amovfx · 2023-06-09T18:24:39Z

-NewNetwork now has an input to create/attach to an external docker network
-NetworkActions now has a modal for the user to
create/attach/clear to an external docker network.

Closes #(issue number goes here)
#525

Description

Allows for polar to attach or create an external docker network.

Steps to Test

Create a new network.
Fill out the Docker External Network field
Check the docker-compose file or docker network ls
In a current network, go to docker options in the hamburger menu
of NetworkActions.
In the modal, name your docker network. Hit okay
Check the docker-compose file or docker network ls.

A status icon is also visible.

amovfx · 2023-06-09T18:25:18Z

ANd just a heads up, this is a prototype, I dont have any tests or anything written yet.

jamaljsr · 2023-06-14T15:01:44Z

Apologies for the delay in reviewing this. I've been traveling. I'll test this out and provide feedback by the end of the week. Thanks for tackling this issue.

amovfx · 2023-09-18T00:50:46Z

Hey, @jamaljsr could I get a quick review on its current roughed in ui? I'm worried it might be going out of scope.

Questions:
Should new network have the ability to create or attach a docker network. While remaining unattached and having used this feature already on my current project, this is something I like.

Is this badge a good idea?

Docker options, I think this is appropriate place for them while providing a place extend docker features.

jamaljsr · 2023-09-19T20:17:05Z

Hey @amovfx thanks for the updates. I'll take a look at this in the next few days.

jamaljsr · 2023-09-23T15:51:20Z

I have tested the functionality. Great job querying for the list of existing networks. That's a great touch 👌

I do have some suggestions for improvement:

When creating a new network, we should hide the external network field under a collapsible Advanced Options section. Since 99% of users will never use this, I don't think it should be front and center.
I'm not a fan of the badge next to the network name. These messages would look better as alerts on the canvas, similar to this
Adding the Docker Options item to the dropdown menu is great placement. I would just suggest to put it above the Delete option. It's better UX to have delete at the bottom.
I created a new Polar network and specified an existing docker network. Afterwards, I went to the Docker Options modal but the network I chose was not pre-filled in the dropdown.

amovfx · 2023-09-25T21:57:56Z

Awesome feedback. Will make these adjustments asap. Thanks.

amovfx · 2023-09-26T22:16:03Z

Made these changes if you want to give them a look over and give the thumbs up. I'm going start doing a code review and a git squash. This one got pretty hairy so I'll probably need quite a bit of feedback on the review to bring it up to spec.

This update allows for creating and/or attaching of docker external networks

amovfx · 2023-09-27T23:15:20Z

Hey @jamaljsr, Looking forward to your review.

I think this is the start of docker functionality. I have a couple branches of this PR to prototype some behavior that is helping my project that is interacting with docker more. Right now docker service actions are being called from the network store. This has me wondering if a store/model/docker.ts is going to be required.

jamaljsr

Thanks so much for implementing this. I really like that you fetch the existing docker networks to make it easy to choose the correct one.

Great work on the implementation. I have a bunch of comments on the code, but I don't think there are any major changes.

You have a conflict with the yarn.lock file. I'd suggest reverting your changes there. I also noticed there are still some linter errors that pop up when running yarn tsc.

jamaljsr · 2023-10-06T03:11:20Z

.vscode/settings.json

The changes to this file aren't necessary for this PR. Was this committed by accident?

jamaljsr · 2023-10-06T03:30:22Z

package.json

@@ -27,7 +27,7 @@
    "prebuild": "tsc -p electron/tsconfig.json && yarn theme",
    "prepare": "husky install",
    "release": "standard-version --no-verify --skip.commit --skip.tag",
-    "start": "rescripts start",
+    "start": "rescripts --openssl-legacy-provider start",


I'm not sure why this change was needed for you, but I got this error when trying to start the app with it:

node: bad option: --openssl-legacy-provider

When I removed this option it worked fine.

jamaljsr · 2023-10-06T03:37:22Z

yarn.lock

Did you need to nuke the yarn.lock file? It's running fine on my machine without these changes.

jamaljsr · 2023-10-06T03:41:49Z

src/lib/docker/composeFile.ts

@@ -37,10 +46,19 @@ class ComposeFile {
  constructor(id: number) {
    this.content = {
      version: '3.3',
-      name: `polar-network-${id}`,
+      name: polarNetworkName(id),


I would refrain from extracting this into a util function until it's used in multiple places.

jamaljsr · 2023-10-06T03:42:05Z

src/lib/docker/composeFile.ts

      services: {},
    };
  }
+  setExternalNetworkName(name: string | undefined) {


nit: add a space above this line.

jamaljsr · 2023-10-06T04:34:23Z

src/components/network/DockerNetworkModal.spec.tsx

+        });
+      });
+      describe('when the input is invalid', () => {
+        it('should isabled the OK button', async () => {


typo: isabled

jamaljsr · 2023-10-06T04:34:55Z

src/components/designer/SidebarCard.tsx

+// @jest-ignore
+


These empty lines aren't necessary.

jamaljsr · 2023-10-06T04:36:08Z

src/components/common/StatusTag.tsx

  const statusColors = {
    [Status.Starting]: 'blue',
    [Status.Started]: 'green',
    [Status.Stopping]: 'blue',
-    [Status.Stopped]: statusTag.stopped,
+    [Status.Stopped]: 'red',


This shouldn't be changed to red because stopped is not an error state.

jamaljsr · 2023-10-06T04:37:30Z

src/components/common/StatusBadge.spec.tsx

@@ -3,33 +3,37 @@ import { Status } from 'shared/types';
 import { renderWithProviders } from 'utils/tests';
 import StatusTag from './StatusTag';

-describe('StatusBadge Component', () => {
+describe('StatusTag Component', () => {


Why change this StatusBadge.tsx file to test the StatusTag component? Did you mean to create a new test file?

jamaljsr · 2023-10-06T04:42:49Z

src/components/common/DockerNetworkName.tsx

+interface Props {
+  name: string;
+  defaultValue?: string;
+  validateCallback?: (value: boolean) => void;


Instead of bubbling up the validation status via props, you could use rules prop to specify a custom regex pattern. This would prevent the form from submitting, display the error message inline, and change the border color of the input field.

<Form.Item rules={[{ pattern: /^[a-zA-Z0-9][a-zA-Z0-9_.-]{1,63}$/, message: l('helpInvalid') }]}>

amovfx mentioned this pull request Jun 9, 2023

Feature Request: Define persistent docker network #525

Open

jamaljsr self-requested a review September 19, 2023 20:15

feat: enable external docker compose networks

2a4137f

This update allows for creating and/or attaching of docker external networks

amovfx force-pushed the feat/persistent-docker-network branch from 9335722 to 2a4137f Compare September 27, 2023 22:12

amovfx marked this pull request as ready for review September 27, 2023 23:12

jamaljsr requested changes Oct 6, 2023

View reviewed changes

jamaljsr force-pushed the master branch from ccaf414 to a54db3d Compare February 6, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/persistent-docker-network #734

Feat/persistent-docker-network #734

amovfx commented Jun 9, 2023 •

edited

amovfx commented Jun 9, 2023

jamaljsr commented Jun 14, 2023

amovfx commented Sep 18, 2023

jamaljsr commented Sep 19, 2023

jamaljsr commented Sep 23, 2023

amovfx commented Sep 25, 2023 •

edited

amovfx commented Sep 26, 2023

amovfx commented Sep 27, 2023

jamaljsr left a comment

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

jamaljsr Oct 6, 2023

Feat/persistent-docker-network #734

Are you sure you want to change the base?

Feat/persistent-docker-network #734

Conversation

amovfx commented Jun 9, 2023 • edited

Description

Steps to Test

amovfx commented Jun 9, 2023

jamaljsr commented Jun 14, 2023

amovfx commented Sep 18, 2023

jamaljsr commented Sep 19, 2023

jamaljsr commented Sep 23, 2023

amovfx commented Sep 25, 2023 • edited

amovfx commented Sep 26, 2023

amovfx commented Sep 27, 2023

jamaljsr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amovfx commented Jun 9, 2023 •

edited

amovfx commented Sep 25, 2023 •

edited