Do not panic on string conversions in yaml unmarshaling.

[imikushin]

Do not panic on string conversions in yaml unmarshaling.

Signed-off-by: Ivan Mikushin i.mikushin@gmail.com

[vdemeester]

Sounds fair 😉
Does the test testing that it does not panic ? :P

[dnephin]

If I understand correctly, this will treat true as 'true', which I don't think is correct.

Does it also cast all numbers to strings?

Wouldn't this be better handled by #99 ?

[imikushin]

@vdemeester The test does test unmarshaling does not panic.
Try running it w/o the changes to types_yaml.go!

@dnephin It does cast everything to string, instead of raising panics. I was thinking: the input file is text (readable and writable by humans) and the target datatype is string, so why not just use the string representation of the value.

On the other hand, the orthodox docker-compose does issue an error, and winter #99 is coming.
I'll update to return errors instead of throwing panics.

[imikushin]

Returning errors on incorrect datatypes now.

[imikushin]

@vdemeester @dnephin @ibuildthecloud I believe I've addressed the comments so far.

[dnephin]

This block (including some of the existing lines) are repeated in quite a few places. Could we make this a function?

func ToStringSlice(value []interface{}) ([]string, error) {
    ...
}

Then many of these can be just:

if s.parts, err = BuildParts(value); err != nil {
    return err
}

[imikushin]

Done.

[dnephin]

I'm not that familiar with this yaml parser. Why is it necessary that these values are cast to strings? Aren't they already strings? Is it trying to deal with cases where yaml treats them as numbers instead of strings?

[imikushin]

Parsed values can be anything: bool, integer, string. Consider this YAML list value, for example:

[1, "sick", list, of, false, assumptions]

Suppose, we have our own data type that uses []string internally. When the parser is unmarshaling the given value and calling UnmarshalYAML(tag string, value interface{}) method, here's what it's putting into the value argument:

[]interface{}{1, "sick", "list", "of", false, "assumptions"}

We've got an int, and a bool along with strings. So we have two options:

• assert that each of the elements is a string, by using the type assertion v.(string), and return an error otherwise (or throw a panic if we don't explicitly check the assertion is successful)
• use a string representation of each value, by using fmt.Sprint(v), and BTW this is what is used by the parser itself to unmarshal to []string and what I've been proposing in the first place

[imikushin]

Done again.

[dnephin]

assert that each of the elements is a string

I think this is what the strict yaml validation should be doing for us.

[imikushin]

I think this is what the strict yaml validation should be doing for us.

Schema validation makes sure each element of the []interface{} value is a string.
Two things:

• throwing panics is not nice when the API suggests an error can be returned
• We don't have schema validation yet. When we have it, we'll just drop the if !ok {...} part which will become redundant.

So, I propose checking for type casting errors (that's what this code is doing now) and drop these checks when the strict yaml validation is in.
Alternatively, drop them now and face the panics OR coerce all values to strings via fmt.Sprint(v) (my original proposal).

Pick one 🎲

[imikushin]

@vdemeester Opinions welcome 😄

[ibuildthecloud]

I'm trying to think what is the correct behavior here. docker-compose doesn't actually blow up if you pass

labels:
  foo: true

The parser succeeds but then the Docker API itself will then blow up with some go marshaling API error. So I kind of lean towards doing the same thing. This change makes our parser more pedantic than docker-compose itself. @dnephin WDYT?

[dnephin]

I think in 1.5.2, it should give a warning when a boolean is used. Previously we were only warning for booleans in the environment field.

I'm happy with more pedantic, as I think that's the direction that compose is going.

[ibuildthecloud]

@dnephin It's a hard pill to swallow for me that foo: true is not supported. Is there docker-compose issue I can comment on or see more context why you guys have decided to go this route? This just seems like a really bad user experience. The same applies to foo: 1. It seems like simple scalars we can safely convert to a string.

[ibuildthecloud]

@dnephin I've been reading through the yaml spec and understand now the complexities.

[ibuildthecloud]

LGTM

[imikushin]

@vdemeester Merge? 😉

[vdemeester]

@imikushin yes 😝
LGTM 🐼

[imikushin]

Thx!

On Tue, Dec 8, 2015, 14:52 Vincent Demeester notifications@github.com
wrote:

Merged #108 #108.

—
Reply to this email directly or view it on GitHub
#108 (comment).