Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow creation from dict #795

Merged
merged 19 commits into from
Jul 23, 2019

Conversation

oz123
Copy link
Contributor

@oz123 oz123 commented Mar 26, 2019

This is a fix for #722.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2019
kubernetes/utils/create_from_yaml.py Outdated Show resolved Hide resolved
kubernetes/utils/create_from_yaml.py Outdated Show resolved Hide resolved
kubernetes/utils/create_from_yaml.py Outdated Show resolved Hide resolved
kubernetes/utils/create_from_yaml.py Outdated Show resolved Hide resolved
@micw523
Copy link
Contributor

micw523 commented Mar 26, 2019

Also you may want to add a test case each for dict type creation (if you'll be doing it) and for creating from string.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2019
@oz123
Copy link
Contributor Author

oz123 commented Mar 26, 2019

Also you may want to add a test case each for dict type creation (if you'll be doing it) and for creating from string.

Done. Thanks for your suggestions.

@@ -30,7 +30,7 @@ def create_from_yaml(
Perform an action from a yaml file. Pass True for verbose to
print confirmation information.
Input:
yaml_file: string. Contains the path to yaml file.
yaml_file: string. Contains yaml string or a path to yaml file.
Copy link
Contributor

@juliantaylor juliantaylor Mar 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be a bit odd to expect a yaml string as input.
Wrapping preexisting strings into file-like objects is normally done by the caller the reasoning being if you have already have stream you have to read it into memory just for the function to convert it back to a memory stream.
Wrapping a pre-existing memory object into a stream on the otherhand is cheap as it just boils down to a memory view.

The usual interface is file path or file-like object. a file-like is also still acceptable for the argument name yaml_file, while that name does not really fit for a string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we want to support string input is that kubectl create -f - takes a stdin input. I know it's kinda weird since bash does not really have dicts, but the functionality is there...

https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands#create

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that kind of my point stdin is not a string, stdin is a file-like object
so to use this function like kubectl create -f - you call create_from_yaml(sys.stdin)
the mapping of a potential - commandline argument to stdin should be done by the caller

@oz123
Copy link
Contributor Author

oz123 commented Apr 4, 2019

@micw523 any news on this? The master branch is advancing and I am afraid this PR will rot soon.

@micw523
Copy link
Contributor

micw523 commented Apr 4, 2019

@oz123 Is it possible if you fast forward the submodules with respect to the upstream? Some things are broken in the CI and I think it's due to the updates in the submodules.

 This fixes of all tests and keeps the original API.

 It's the users responsiblility to do something the execptions
 when using `create_from_dict`, no air bag or breaks are supplied here.
@oz123
Copy link
Contributor Author

oz123 commented Apr 5, 2019

@micw523 I have fixed most of the tests. Now just one test is failing (TestUtils.test_create_apps_deployment_from_yaml_obj) will look into this in the coming days.

@micw523
Copy link
Contributor

micw523 commented Apr 5, 2019

I checked the CI history and it seems that you have a type error problem when you used the stringIO.

@oz123
Copy link
Contributor Author

oz123 commented Apr 5, 2019

I checked the CI history and it seems that you have a type error problem when you used the stringIO.

Thanks, I will look at it this weekend. I had some other issues I missed when I pushed the code.
My last commits fixed those, but it's definitely not ready.

I have notices that sometimes when the TestSuite fails it doesn't clean resources properly.
Is it worth opening an issue for that? I'd like to fix this.

We create a deployment and do the following:

```
self.assertIsNotNone(dep)
```

Which does not fail, and then the code proceeds to deletion
and fails with a 404 execption in 80% of the time, but sometimes
it works. The deployment is there, but for some reason not available for
deletion.
Travis CI also showed inconsitent behaviour on this. Python3.5 passed
but all other version failed.
With this commit we wait for the deployment to become available for
deletion and only then continue.
Using `nginx-app` deployment multiple times, is problematic because we get
conflicts or not found error.

Instead of trying to handle all cases, explicit different names
are used now. The tests now runs more reliably
This is embarrassing.
@micw523
Copy link
Contributor

micw523 commented Jun 20, 2019

Hey @roycaihw, I think k8s 1.15 is released and I'm not sure if the current minikube is ready for it.

@micw523
Copy link
Contributor

micw523 commented Jun 20, 2019

@oz123 @roycaihw I just checked - the current minikube only supports up to v1.14.3. What do you want to do?

@oz123
Copy link
Contributor Author

oz123 commented Jun 20, 2019

@micw523 I am not familiar with the test suite, can one specify the k8s version, and temporarily pin it to 1.14.X?

@roycaihw
Copy link
Member

roycaihw commented Jun 20, 2019

@micw523 Thanks. Please feel free to send a pull to pin the k8s version to 1.14

@oz123
Copy link
Contributor Author

oz123 commented Jun 21, 2019

@micw523 Thanks. Please feel free to send a pull to pin the k8s version to 1.14

@roycaihw see #856.

@oz123 oz123 changed the title Allow create from string or from dict Allow creation from dict Jun 25, 2019
@oz123
Copy link
Contributor Author

oz123 commented Jun 25, 2019

/assign @roycaihw, all tests pass. Can you check whether this is now acceptable?

@oz123
Copy link
Contributor Author

oz123 commented Jul 4, 2019

@micw523 @roycaihw can this please moved forward , this is really wanted. See also #770 which tries to achieve the same thing.

@micw523
Copy link
Contributor

micw523 commented Jul 23, 2019

This LGTM if you remove the changes in kube-init.sh since we already merged that.

The current PR no longer support creating from string
This follows up on the addition to create_from_yaml, the behavior is the
same.
@oz123
Copy link
Contributor Author

oz123 commented Jul 23, 2019

@micw523, I removed the commit and forced push the branch again. This should now be legit.

I just realized that this causes the tests to fail. The whole point of git is that identical commits are not merged twice into the branch...
The tests should now pass with the changes from master committed to this branch.

Also, now the tests pass, beside the functional test for Python3.6, but that's not a Python issue. Minikube seems to not start (fragile test suite?)

@micw523
Copy link
Contributor

micw523 commented Jul 23, 2019

See #890 for the test problems - the default config file requests 2 CPUs to run and sometimes Travis only provides 1. I don't know why this is happening since we didn't have a problem for quite a while using minikube, but we probably want to configure minikube to use only 1 CPU in the future.

@micw523
Copy link
Contributor

micw523 commented Jul 23, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2019
@roycaihw
Copy link
Member

I restarted the test job and it passed. Seems to be a flake

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oz123, roycaihw

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2019
@k8s-ci-robot k8s-ci-robot merged commit b8cb6e8 into kubernetes-client:master Jul 23, 2019
@tdmalone
Copy link

Thanks for this! Bit of a kubernetes noob here, but could I ask when this might be released? (I realise I can install from source in the meantime, but keen to stick to released versions if possible)

@oz123 oz123 deleted the util-improvements branch August 28, 2019 12:35
@roycaihw
Copy link
Member

@tdmalone This will be included in v11.0.0a1, which is coming out soon. See https://github.com/kubernetes-client/python/pull/931/files#diff-4ac32a78649ca5bdd8e0ba38b7006a1e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants