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

Add kubectl create -f like feature #655

Merged
merged 1 commit into from Nov 1, 2018

Conversation

micw523
Copy link
Contributor

@micw523 micw523 commented Oct 22, 2018

This PR addresses #504.

Created a module kubernetes/utils with a function create_from_yaml(). Implemented with getattr().

e2e unit tests were added to test the functionalities on app_v1 / extension_v1beta1 (deployment), core_v1 (pod, service). The test yaml files are from kubernetes.io are included under e2e_test/test_yaml.

A minimal working example is added under examples/ folder.

@micw523
Copy link
Contributor Author

micw523 commented Oct 22, 2018

/assign @roycaihw
Could you please take a look?

@micw523
Copy link
Contributor Author

micw523 commented Oct 22, 2018

Fixes #349

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 22, 2018
@nakulpathak3
Copy link

This is awesome, thank you so much. Do you think it would be possible to have one for creating job from job_spec.yaml file as well? I don't see it in the code right now

@micw523
Copy link
Contributor Author

micw523 commented Oct 25, 2018

@nakulpathak3 From what I implemented I would expect a call to batch/vxx kind:Job to work. I did not implement an e2e test for it though.

Copy link
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature! I left a few nits, and one substantive comment around creating cluster-scoped resources.

@@ -0,0 +1,31 @@
# Copyright 2016 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

s/2016/2018

similar in other places under utils/


import yaml

from kubernetes import client, config, utils
Copy link
Member

Choose a reason for hiding this comment

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

I think we can drop client and yaml imports?


def test_app_yaml(self):
k8s_api = utils.create_from_yaml(
"kubernetes/e2e_test/test_yaml/app.yaml",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call the yaml files apps-deployment.yaml and extensions-deployment.yaml. We can use singular for the Kind, but usually we don't have singular v.s. plural for Group

self.assertEqual("apps/v1beta1",
k8s_api.get_api_resources().group_version)
deployments = k8s_api.list_namespaced_deployment(
namespace="default").items
Copy link
Member

Choose a reason for hiding this comment

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

You could use GET instead of LIST to test the creation:

def read_namespaced_deployment(self, name, namespace, **kwargs):


from kubernetes import client

def create_from_yaml(yaml_file, verbose=0, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

nit: please use verbose=False and verbose=True

dep_namespace = dep["metadata"]["namespace"]
else:
dep_namespace = "default"
resp = getattr(k8s_api, "create_namespaced_{0}".format(action_type.lower()))(
Copy link
Member

Choose a reason for hiding this comment

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

We also need to be able to create cluster-scoped resources (e.g. create a namespace)

Copy link
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

LGTM in general. I left a few nits. Please squash the commits before submitting.


def create_from_yaml(k8s_client, yaml_file, verbose=False, **kwargs):
"""
Perform an action from a yaml file. Pass 1 for verbose to
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/1/True

with open(path.abspath(yaml_file)) as f:
yml_object = yaml.load(f)
#TODO: case of yaml file containing multiple objects
api_type, _, api_version = yml_object["apiVersion"].partition("/")
Copy link
Member

Choose a reason for hiding this comment

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

Please use group, version and kind instead of api_type, api_version and action_type, following the Kubernetes API convention: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md

# Decide which namespace we are going to put the object in,
# if any
if "namespace" in yml_object["metadata"]:
dep_namespace = yml_object["metadata"]["namespace"]
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/dep_namespace/namespace

@roycaihw
Copy link
Member

roycaihw commented Nov 1, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: micw523, 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 Nov 1, 2018
@k8s-ci-robot k8s-ci-robot merged commit 2dafb4b into kubernetes-client:master Nov 1, 2018
:param str dry_run: When present, indicates that modifications should not be persisted. An invalid or unrecognized dryRun directive will result in an error response and no further processing of the request. Valid values are: - All: all dry run stages will be processed
"""

with open(path.abspath(yaml_file)) as f:
Copy link

Choose a reason for hiding this comment

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

path.abspath(yaml_file)
will crash on str while yaml.load(str) can load the yaml from a string.
very useful if you need to change the yaml dynamically

@govindKAG
Copy link

Can you create argo workflows with this ? because I get
module 'kubernetes.client' has no attribute 'Argoproj.ioV1alpha1Api'module 'kubernetes.client' has no attribute 'Argoproj.ioV1alpha1Api'

@micw523
Copy link
Contributor Author

micw523 commented Feb 20, 2019

Is it a CRD? If so see #740. This is in my to do list, but these features have to come one at a time.

@govindKAG
Copy link

Is it a CRD? If so see #740. This is in my to do list, but these features have to come one at a time.

yeah it is a CRD. I am not sure how I missed that issue, thanks.

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

6 participants