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

Add port command #52

Merged
merged 1 commit into from
Apr 11, 2019
Merged

Add port command #52

merged 1 commit into from
Apr 11, 2019

Conversation

LarsKumbier
Copy link
Contributor

Adds the port command, closes #51

@AlexZeitler AlexZeitler merged commit f621154 into PDMLab:master Apr 11, 2019
@AlexZeitler
Copy link
Contributor

AlexZeitler commented Apr 11, 2019

@LarsKumbier Thanks for your contribution. It's published on npm as 0.15.0

@stefanmeschke
Copy link

Thanks for this feature.
I think it would be very nice, if the port command returns only the port and not the whole address. And it is missing in the docs. :)

Do you agree? Should I raise a PR?

@AlexZeitler
Copy link
Contributor

@stefanmeschke Makes sense to me.

@LarsKumbier Would this break something for you aside from only getting the port?
@Steveb-p Any thoughts?

@Steveb-p
Copy link
Contributor

@AlexZeitler usually I believe we do not "enhance" original docker-compose responses. In this case, reading the port from the output is rather trivial, so introducing a special case might not be worth it.

After all, all you need to do to read the port from the output would then be as simple as

const output = "0.0.0.0:2181"
const pos = output.indexOf(":")
const port = output.slice(pos + 1)

@LarsKumbier
Copy link
Contributor Author

yeah, but it get's more complicated with ipv6, error-messages of docker-compose, docker not reachable for whatever reason... string-parsing is just one of the things that is bound to break somewhen.

I think it would be a good addition to get a more structured output in this package to any of the commands, because everyone using this will currently write the same code to do the same things.

For this specific command, an output like this would be more appropriate:

{
    "address": "0.0.0.0",
    "originalPort": "33165"
}

So yeah, I'd like to see a generally changed output format like this:

{
  "raw": "<output>",
  "result": { "address": "0.0.0.0", "originalPort": "33165" }
}

@AlexZeitler
Copy link
Contributor

@LarsKumbier That's exactly what I had in mind after reading the response from @Steveb-p. I think we should embrace this for each command/result.

We should use interfaces for result which would allow users of the lib to use TypeScripts declaration merging if a property on result is missing due to newer docker-compose(the underlying tool) version.

I think it would be a good addition to get a more structured output in this package to any of the commands, because everyone using this will currently write the same code to do the same things.

For this specific command, an output like this would be more appropriate:

{
    "address": "0.0.0.0",
    "originalPort": "33165"
}

So yeah, I'd like to see a generally changed output format like this:

{
  "raw": "<output>",
  "result": { "address": "0.0.0.0", "originalPort": "33165" }
}

@stefanmeschke
Copy link

I really like the idea of

{
  "raw": "<output>",
  "result": { "address": "0.0.0.0", "originalPort": "33165" }
}

To be backward compatible it could be something like this:

export interface IDockerComposeResult<T> {
  exitCode: number | null;
  out: string;
  err: string;
  result: T;
}

@AlexZeitler
Copy link
Contributor

Yes. @stefanmeschke Please create a new issue and copy your suggestion and your last answer from here as a starting point to it.

Also please refer to this PR so we have everything tracked.

We should also define what result is in case of err.

@Steveb-p
Copy link
Contributor

@stefanmeschke when output was not an address & port, what did you receive?

@AlexZeitler @stefanmeschke democratically I'm outvoted I guess :) and I do see the merit of what you want to do.

Can we open a new issue so we can check each and every call to docker-compose and decide when we're going with additional processing for them?

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.

Get dynamic port
4 participants