Issue #2730 - better handle types for expanded variables

[tiry]

Here is a changeset to handle automatic cast based on the types defined in the jsonschema.
I added test for my use case as well as for #2750.

My python is pretty rusted, but it seems to work as expected ... let me know what you think.

[dnephin]

I'm not sure why this is necessary. On windows I would expect os.path to return the correct paths.

[tiry]

the if sys.platform == "win32": was here in the code I moved ...

[dnephin]

Ah yes, I saw that, but I'm not sure why it's there either. This is mostly just a question, it may be necessary. If you can remove it and the appveyor tests pass (they run on windows) then I think it's safe to remove.

[tiry]

I'll try to integrate the feedback asap and will resubmit.

[dnephin]

This seems overly complex. I think supporting arrays is probably the most contentious part of this feature. I think we should try to keep it as straightforward as possible.

How about:

return json.loads(interpolated_value)

Although, I wonder if it should actually be yaml.safe_load() since the config is yaml format. Using yaml would support json as well.

[tiry]

As I said I have not written python for years, but it seems that json.loads does not work with an Array.
Looks like I am not the only one to face the issue
=> http://stackoverflow.com/questions/10973614/convert-json-array-to-python-list

However, it may be just because of issue between simple quotes (') and double quotes (")

[dnephin]

I think that's it. json only supports double quotes for strings.

[dnephin]

Thanks for the PR! I think it's on the right track.

We'll need some time to discuss it for the 1.7 release, so it might be another couple weeks before we can give any more design feedback.

[tiry]

What is the preferred process on your side :

• I amend my previous commit
• I simply add more commits (and, may be, you squash then before merge)
• I submit an other PR

NB : I know this is a kind a "religious" question, so I prefer to ask before doing something inappropriate :)

[dnephin]

New commits are fine. You may be asked to squash at the end, right before merging. Or you could also squash them now if you think it's warranted.

[tiry]

Here is a new version.
I tried to incorporate most of the comments.

I say most, because while changing the code I realized that some cases where not handled, so I added more tests and adapted the implementation.
As a result, the _cast_interpolated is not as simple as I would have hoped ...

[tiry]

Not sure I understand the problem on the Jenkins build: it is related to my changes or can it come from other changes ?

[dnephin]

They look unrelated

[tiry]

Ok, should I do something to restart the build / check ?

[tiry]

Looks like I need to rebase my changes ... will do it when I find the time

[flx42]

Any progress @tiry? Since this PR is solving #2750, it would be great for nvidia-docker. Thank you for your work!

[tiry]

I did a rebase (+fixes) and everything seems to work on my side ... let's see what you CI says !

[tiry]

It does not look like the build failure is related to my code.
Can someone restart the job (since I do not have the permission to do it myself on Jenkins), or should I do an empty commit ?

[flx42]

I don't know how the CI works for docker-compose, but usually on GitHub you can just git commit --amend --reset-author and then git push -f (force push the modified commit on your branch) to trigger new checks.

[tiry]

Fair enough, I was just looking for a cleaner option.

[tiry]

Failed again so the error does not seem to be transient.
My understanding is that the problem is not related to my code change, if someone on docker-compose could take a look this would be helpful.

Thank you !

[dnephin]

yes, the failures are unrelated, i've started another build now that the issue should be fixed.

[tiry]

Great, let me know if you have further feedback !

[tiry]

I could rebase once more, but I would need to have some feedback from you guys otherwise this is kind of pointless.

[dnephin]

Yup, don't worry about rebasing just yet, we'll need to look over it again

[ruffsl]

Any status on review of this PR? I'm here coming from the same nvidia-docker related camp (#2750 and NVIDIA/nvidia-docker#39) as @flx42 .

[shin-]

#5291