utils and utils.create_from_yaml improvements

[oz123]

As discussed in #722.

Seems this had quite some positive feedback.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: oz123
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yliaog

If they are not already assigned, you can assign the PR to them by writing /assign @yliaog in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[juliantaylor]

I don't think one needs to explicitly reference yaml here as what the function wants is a kubernetes manifest in python dictionary form
though it probably also does not harm as yaml is the most common representation form of that data.

[oz123]

this is why I added create_from_map

[roycaihw]

/cc @micw523

[micw523]

I think I'm still holding my original position in #722: we don't need many function names. We should be able to do everything in create_from_yaml (potentially with an extra input parameter), since the word yaml isn't specific to a file anyway. If absolutely needed we can make an alias on create_from_yaml.

Also, we're pushing to merge 673, so it's likely that you'll need a rebase soon...

[micw523]

See comments.

[oz123]

I think I'm still holding my original position in #722: we don't need many function names. We should be able to do everything in create_from_yaml (potentially with an extra input parameter), since the word yaml isn't specific to a file anyway. If absolutely needed we can make an alias on create_from_yaml.

Also, we're pushing to merge 673, so it's likely that you'll need a rebase soon...

I agree with you about creating an alias. IMHO it's better to be explicit . Since "yaml" isn't necessary a file it's better to specify when expect a file\stream and specify when we don't.

Concerning backwards compatibility, I now suggest the following, check if yaml_path is a file on the filesystem, if it is, proceed as in the past. If it is a string, try and parse it to yaml object.

[oz123]

I checked again:

$ grep -nr adal kubernetes | grep -v \.pyc
kubernetes/base/config/kube_config.py:24:import adal
kubernetes/base/config/kube_config.py:223: context = adal.AuthenticationContext(

jwt is a dependency of adal, and I'm not sure adal is specifically required here.
I removed jwt. How do you want me to proceed with adal?

[micw523]

Which branch did you fork it from? If you forked it from the current master I’ll need to do some investigation.

Adal as a requirement got moved to optional in a previous PR so that might be the problem.

[oz123]

Which branch did you fork it from? If you forked it from the current master I’ll need to do some investigation.

Adal as a requirement got moved to optional in a previous PR so that might be the problem.

I forked from the master on a fresh clone I did yesterday.

[micw523]

I just had a fresh fork from the master branch. adal is not required for testing. Is it breaking anything that you submitted?

[micw523]

I checked your past Travis builds. It doesn't seem like adal is affecting you. Please remove the added packages.

[micw523]

/assign

[oz123]

I checked your past Travis builds. It doesn't seem like adal is affecting you. Please remove the added packages.

done. I completely removed that commit.

[oz123]

@micw523 the new master branch includes create_from_yaml_single_item which obsoletes my work.
I think a lot of people would appreciate a more explicit function name, because create_from_yaml is still misleading. A better name would have been create_from_yaml_file. In order to keep backward compatabilty create_from_yaml_file could be added in the future as a thin wrapper around create_from_yaml_file and if people use create_from_yaml use a warning, but still read the file and parse the yaml.
My two cents.

[micw523]

I’m not sure. The create_from_single_item is there for a multi-resource yaml and / or yaml lists. It is NOT imported into the package nor you can use it directly with inputs from a string if your yaml string / dict contains a list type or multiple resources. This idea is not obsolete if these cases are handled.

Do you still want to work on this, or you’re going to leave it in my hands?

[oz123]

@micw523 I would prefer starting on a clean branch and a clean PR. This has become too opaque for me. Do you mind if I submit a new PR?

[micw523]

That’s cool, go for it :)