Skip to content
This repository has been archived by the owner on Jun 11, 2021. It is now read-only.

Add custom volumes as an option #4401

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

aaomidi
Copy link

@aaomidi aaomidi commented Dec 17, 2018

#1923 #4399

These issues were closed with no resolution. I decided to go ahead and make these changes.

The code changes how the volume section is handled to be the same as the environment variables. Allow custom volumes and deleting of any existing volumes.

This PR can be further extended to ensure that if a volume change broke Docker, it can revert back to a cached list of mounts.

@aaomidi
Copy link
Author

aaomidi commented Dec 17, 2018

Minor UI changes to make this work:

image

@aaomidi
Copy link
Author

aaomidi commented Dec 20, 2018

Hey @jdrouet can you take a look at this PR?

@Marahin
Copy link

Marahin commented Jan 15, 2019

Is this feature forgotten?

@aaomidi
Copy link
Author

aaomidi commented Jan 16, 2019

@Marahin I'm ready here to provide any updates to get this into kitematic. Unfortunately the contributors are showing bad faith by simply ignoring it.

Copy link
Contributor

@jdrouet jdrouet left a comment

Choose a reason for hiding this comment

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

didn't check the feature by itself yet but there are some changes to make in your PR ;)

src/components/ContainerSettingsVolumes.react.js Outdated Show resolved Hide resolved
src/components/ContainerSettingsVolumes.react.js Outdated Show resolved Hide resolved
} else {
icon = <a onClick={this.handleRemoveVolume.bind(this, index)} className="only-icon btn btn-action small"><span
className="icon icon-delete"></span></a>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably take this piece of code out of this function

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure where to place it. Could I leave this one up to you? I was following the same code style as the other ContainerSetting pages:

if (index === this.state.env.length - 1) {
icon = <a onClick={this.handleAddEnvVar} className="only-icon btn btn-positive small"><span className="icon icon-add"></span></a>;
} else {
icon = <a onClick={this.handleRemoveEnvVar.bind(this, index)} className="only-icon btn btn-action small"><span className="icon icon-delete"></span></a>;
}

src/components/ContainerSettingsVolumes.react.js Outdated Show resolved Hide resolved
@Marahin
Copy link

Marahin commented Jan 16, 2019

I just wanted to drop-in and say thank you @jdrouet for finding time to check this out, it's something I'm looking forward to see in Kitematic. 👍

@aaomidi
Copy link
Author

aaomidi commented Jan 16, 2019

Thank you @jdrouet!

@benjamingr
Copy link

Hey, is there any progress on reviewing this @jdrouet ? How can we help?

cc @stereokai

@Cyclic
Copy link

Cyclic commented Feb 16, 2019

Ship it! 🚀Please 👍

@Chrisdowson
Copy link

come on, we need this feature in Kitematic

@stereokai
Copy link

@jdrouet so what's happening with this PR?

@Marahin
Copy link

Marahin commented Mar 29, 2019

🙌

@ms3rgio
Copy link

ms3rgio commented Apr 7, 2019

🎉

@stereokai
Copy link

@aaomidi Your PR has been approved

@stereokai
Copy link

@aaomidi @jdrouet This PR has been approved, what's blocking a release? Can we help in any way?

@aaomidi
Copy link
Author

aaomidi commented May 7, 2019

Nothing on my side unfortunately.

Trackhe added a commit to Trackhe/kitematic that referenced this pull request Sep 28, 2019
Merges Pull Request: docker#3462 .. docker#3692 .. docker#4401 .. docker#5306

They are Fix Login. and adding stuff.

temporarily simplifying the building. and remove the test.
Trackhe added a commit to Trackhe/kitematic that referenced this pull request Sep 28, 2019
Merges Pull Request: docker#3462 .. docker#3692 .. docker#4401 .. docker#5306

They are Fix Login. and adding stuff.

temporarily simplifying the building. and remove the test.

add building on TC with tag. and pushing release to github.
@damianstasik
Copy link

What can be done to get this merged? What are the blockers?

@moos
Copy link

moos commented Jan 19, 2020

https://github.com/DockStation/dockstation fits the bill.

@Hc747
Copy link

Hc747 commented Jan 21, 2020

Bump - pls merge

@ash0080
Copy link

ash0080 commented Mar 3, 2020

What happened on releasing? It has been more than 1 year since the PR pushed !

@eried
Copy link

eried commented Apr 1, 2020

So slow :/

@aaomidi
Copy link
Author

aaomidi commented Apr 1, 2020

If there is interest I'll push a release of the binaries to my fork...

@eried
Copy link

eried commented Apr 1, 2020

If there is interest I'll push a release of the binaries to my fork...

I instinctively checked your repo for releases but https://github.com/DockStation/dockstation was a nice workaround.

With this slow merging I would say everyone should ignore kitematic and support dockstation instead.

@aaomidi
Copy link
Author

aaomidi commented Apr 2, 2020

Yeah I don't think I want to be responsible for maintaining a fork of this :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet