Fix substitution with non-empty env-var#1392
Conversation
|
This should be cherry-picked in the 18.09 branch as well ping @anokun7 @vdemeester @silvin-lubecki PTAL |
vdemeester
left a comment
vdemeester
left a comment
There was a problem hiding this comment.
LGTM 🐸
(but test is red ?)
|
Oh, interesting; I tried my new test, but the existing ones broke 🤔 let me check why |
|
Looks like the "soft default" doesn't actually check for empty values, so that's an existing bug as well (which went unnoticed because we never reached that part). I'll have a look after dinner 😅 |
Due to a typo, substitution would not work if the given
environment-variable was set.
Given the following docker compose file;
```yaml
version: "3.7"
services:
app:
image: nginx:${version:-latest}
```
Deploying a stack with `$version` set would ignore the `$version`
environment variable, and use the default value instead;
```bash
version=alpine docker stack deploy -c docker-compose.yml foobar
Creating network foobar_default
Creating service foobar_app
docker service ls
ID NAME MODE REPLICAS IMAGE PORTS
rskkjxe6sm0w foobar_app replicated 1/1 nginx:latest
```
This patch also fixes "soft default" not detecting empty environment variables,
only non-set environment variables.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
fix_substitution_with_non_empty_value
branch
from
4e1d8ad to
ec3daea
Compare
Codecov Report
@@ Coverage Diff @@
## master #1392 +/- ##
==========================================
+ Coverage 54.88% 54.89% +0.01%
==========================================
Files 293 293
Lines 19428 19434 +6
==========================================
+ Hits 10663 10669 +6
Misses 8089 8089
Partials 676 676 |
|
Fixed; should be good to go now |
| func softDefault(substitution string, mapping Mapping) (string, bool, error) { | ||
| return withDefault(substitution, mapping, "-:") | ||
| sep := ":-" | ||
| if !strings.Contains(substitution, sep) { |
There was a problem hiding this comment.
I guess it can be factorized with hardDefault but I don't see an obvious and/or clean way to do it.
There was a problem hiding this comment.
Yes, I was looking at that, but adding an (e.g.) checkIfSet func(val) bool argument felt like over engineering just to prevent just a few lines of duplicated code.
…on_empty_value Fix substitution with non-empty env-var Signed-off-by: Lifubang <lifubang@acmcoder.com>
5 participants
fixes #1391
Due to a typo introduced in #1249, substitution would not work if the given environment-variable was set.
Given the following docker compose file;
Deploying a stack with
$versionset would ignore the$versionenvironment variable, and use the default value instead;