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

Substitute $HOST and $PORT* in healthcheck command #232

Merged
merged 2 commits into from
May 22, 2017
Merged

Substitute $HOST and $PORT* in healthcheck command #232

merged 2 commits into from
May 22, 2017

Conversation

guilhem
Copy link
Contributor

@guilhem guilhem commented May 19, 2017

No description provided.

{
name: "with ports and host",
args: args{
s: "nc $HOST $PORT0 && nc $HOST $PORT1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is rare scenarion but what if command has escaped variable name? e.g.: echo \$HOST or echo '$HOST'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that can be a problem... but solving this can be very difficult. I found this package https://godoc.org/github.com/mvdan/sh/syntax to parse and "understand" shell but API is not simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomez @dankraw What do you think about it. I'm fine with this simple approach if we document it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janisz documentation done ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 87.474% when pulling 1286839 on guilhem:substitute-command into 64b04b0 on allegro:master.

Copy link
Contributor

@janisz janisz left a comment

Choose a reason for hiding this comment

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

@guilhem Thanks! I really like this feature and I'm in favor of merging it.

We need to add documentation that describes how it works with information when it could fail. Best if we can provide solution that will works the same as in Marathon/Mesos. I mean resolved commands will be the same.

I'm not sure how will this works if user has bridged network, what values has $PORT and $HOST then?

@guilhem
Copy link
Contributor Author

guilhem commented May 19, 2017

I don't know in case of bridged network... I just sure that it doesn't work for them either in current situation ;)

Marathon is running healthcheck shell commands with same execution context than a task... it's really hard to reproduce it.

@janisz
Copy link
Contributor

janisz commented May 19, 2017

You are right. This feature is not working right now so adding it should not damage any Marathon-Consul installation.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 87.474% when pulling 8bf7b2c on guilhem:substitute-command into 64b04b0 on allegro:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 87.474% when pulling 8bf7b2c on guilhem:substitute-command into 64b04b0 on allegro:master.

@janisz janisz merged commit 6a63f9c into allegro:master May 22, 2017
@janisz
Copy link
Contributor

janisz commented May 22, 2017

@guilhem Thanks!

@guilhem guilhem deleted the substitute-command branch May 22, 2017 12:29
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.

5 participants