-
Notifications
You must be signed in to change notification settings - Fork 146
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
Feat/persistent-docker-network #734
base: master
Are you sure you want to change the base?
Conversation
ANd just a heads up, this is a prototype, I dont have any tests or anything written yet. |
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. |
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: Docker options, I think this is appropriate place for them while providing a place extend docker features. |
Hey @amovfx thanks for the updates. I'll take a look at this in the next few days. |
Awesome feedback. Will make these adjustments asap. Thanks. |
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
9335722
to
2a4137f
Compare
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. |
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 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
.
.vscode/settings.json
Outdated
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.
The changes to this file aren't necessary for this PR. Was this committed by accident?
@@ -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", |
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'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.
yarn.lock
Outdated
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.
Did you need to nuke the yarn.lock
file? It's running fine on my machine without these changes.
@@ -37,10 +46,19 @@ class ComposeFile { | |||
constructor(id: number) { | |||
this.content = { | |||
version: '3.3', | |||
name: `polar-network-${id}`, | |||
name: polarNetworkName(id), |
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 would refrain from extracting this into a util function until it's used in multiple places.
services: {}, | ||
}; | ||
} | ||
setExternalNetworkName(name: string | undefined) { |
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.
nit: add a space above this line.
}); | ||
}); | ||
describe('when the input is invalid', () => { | ||
it('should isabled the OK button', async () => { |
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.
typo: isabled
// @jest-ignore | ||
|
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.
These empty lines aren't necessary.
const statusColors = { | ||
[Status.Starting]: 'blue', | ||
[Status.Started]: 'green', | ||
[Status.Stopping]: 'blue', | ||
[Status.Stopped]: statusTag.stopped, | ||
[Status.Stopped]: 'red', |
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.
This shouldn't be changed to red because stopped is not an error state.
@@ -3,33 +3,37 @@ import { Status } from 'shared/types'; | |||
import { renderWithProviders } from 'utils/tests'; | |||
import StatusTag from './StatusTag'; | |||
|
|||
describe('StatusBadge Component', () => { | |||
describe('StatusTag Component', () => { |
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.
Why change this StatusBadge.tsx
file to test the StatusTag
component? Did you mean to create a new test file?
interface Props { | ||
name: string; | ||
defaultValue?: string; | ||
validateCallback?: (value: boolean) => void; |
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.
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') }]}>
Hi @amovfx do you plan on continuing to work on this PR? |
-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.