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

Initial docker command impl #695

Merged
merged 8 commits into from
Sep 28, 2019

Conversation

JonnyWalker81
Copy link
Contributor

Initial implementation of Docker commands. It currently supports the ps and images Docker commands. It should address Issue #619.

@sophiajt
Copy link
Contributor

Cool :) Glad to see someone starting on a docker command.

One suggestion I have here, if you want to wrap the commandline docker and give it a much more Nu-friendly way of working with it, is to add this as a plugin instead of a command.

From there, we can make it an optional feature of Nu (we did this with tree and binaryview as two other examples). Since not everyone will have docker installed, this gives people chance to optionally add the support if they want it.

@JonnyWalker81
Copy link
Contributor Author

That makes sense, I will work on making it a plugin instead of a built in command. Thank you for the feedback, I have enjoyed working on and contributing to this project.

@sophiajt
Copy link
Contributor

@JonnyWalker81 glad to hear it!

We're working on our book for contributors, and there's a chapter on plugins if you're interested: https://github.com/nushell/contributor-book/blob/master/en/plugins.md

@JonnyWalker81
Copy link
Contributor Author

I implemented a Plugin for the Docker command and removed the built-in Docker command I previously proposed. The documentation in the contributor book was helpful, thank you.

Copy link
Contributor

@sophiajt sophiajt left a comment

Choose a reason for hiding this comment

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

I made a few comments on the code.

You may also want to look at how we made some plugins optional based on what features were available (you can see how we've been doing it in the Cargo.toml file). Here, we will probably want users to opt-in, since not everyone will have the docker application on their system.

src/cli.rs Outdated
@@ -248,6 +248,7 @@ pub async fn cli() -> Result<(), Box<dyn Error>> {
whole_stream_command(Next),
whole_stream_command(Previous),
whole_stream_command(Debug),
// whole_stream_command(Docker),
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I had removed the commented out code, sorry about that.

src/commands.rs Outdated
@@ -11,6 +11,7 @@ pub(crate) mod config;
pub(crate) mod cp;
pub(crate) mod date;
pub(crate) mod debug;
// pub(crate) mod docker;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

src/commands.rs Outdated
@@ -79,6 +80,7 @@ pub(crate) use config::Config;
pub(crate) use cp::Cpy;
pub(crate) use date::Date;
pub(crate) use debug::Debug;
// pub(crate) use docker::Docker;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@JonnyWalker81
Copy link
Contributor Author

I added a required-features property in the Cargo.toml file similar to what I saw for other plugins, but I am not fully sure that is correct or if there is something else that is needed also. I removed the commented out code as well.

@sophiajt
Copy link
Contributor

Great!

@sophiajt sophiajt merged commit d52e087 into nushell:master Sep 28, 2019
@iocanel
Copy link

iocanel commented Oct 14, 2019

How can I try this locally?

Tried cargo build --features="docker but it gave me compilation errors?
Do I miss something?
BTW, I am a complete rust noob.

elferherrera pushed a commit to elferherrera/nushell that referenced this pull request Feb 7, 2022
kubouch pushed a commit that referenced this pull request Feb 7, 2022
Hofer-Julian pushed a commit to Hofer-Julian/nushell that referenced this pull request Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants