search

deis / deis

Deis v1, the CoreOS and Docker PaaS: Your PaaS. Your Rules.
https://deis.com/docs/
MIT License
6.04k stars 797 forks source link

Publisher fails to parse HEALTHCHECK_INITIAL_DELAY if the value contains `\r` #5091

Closed NickAb closed 7 years ago

NickAb commented 8 years ago

When using

deis config:push -p staging.env -a my-app

if config file being pushed contains Windows style line endings (\r\n), then \r will be treated as part of the value, which will result in error when trying to parse HEALTHCHECK_INITIAL_DELAY from uploaded config:

2016/09/03 01:47:43 strconv.ParseInt: parsing "10\r": invalid syntax

which results in config push failing with

Creating config... Error:
503 SERVICE UNAVAILABLE
detail: aborting, app containers failed to respond to health check

Which does not give any idea what was actual problem (bad delay value).

Changing file line endings to UNIX style fixes the problem, but shouldn't all line endings be ignored? This will require developers using Windows to careful work with configs, and in case of the error it is hard to diagnose. The \r problem is not obvious as when config is pushed the operation will fail saying that containers failed health-check without any details of actual problem (that Publisher was not able to parse value).

bacongobbler commented 8 years ago

@NickAb since we want to preserve whitespace when someone uses config:set, I think the right way to tackle this would be in the CLI.

If you're up for it, one way we could handle this is by converting all /r/n and /r control characters into /n.

What version are you running?

NickAb commented 8 years ago

I am using 1.13.2 on Windows 8.1 x64.

Maybe CLI should show warning when trying to config:push file with Windows line endings and have an option to auto-convert line endings, how does it sound?

Although handling that in CLI might be a good idea, I still think that Publisher should be able to parse HEALTHCHECK_INITIAL_DELAY even if it contains \r, because Publisher expects the value to be a number and it can ignore any white-space as it is clear that user intended to set a number value for this variable.

Joshua-Anderson commented 8 years ago

@NickAb Since Deis V1 is in LTS mode, and the CLI on windows is experimental, this is unlikely to get a fix for Deis V1. However, patches are always welcome!

I believe this is an problem with Deis Workflow (V2) as well. Windows is officially supported in Workflow, so I'll look into getting a fix for it there. This might also be fixed by switching to .env parsing library, like what was suggested in #4512 for supporting multiline environmental variables.

bacongobbler commented 7 years ago

closing as irrelevant for v2.