Adding ability to delete by selector. Also variable name cleanup and update to use .format. #3840

kwoodson · 2017-04-03T18:25:22Z

No description provided.

kwoodson · 2017-04-03T18:34:14Z

The major changes here:

lib/base.py: These changes allow specifically oc_obj to delete by name or by selector. They propagated to all files as a consequence. PR looks large but change set is small.
I did not like rname as it used to conflict with the way ansible import * was required which conflicted with the name variable. I renamed back to name as we no longer do the import the same way.
I added an integration/oc_obj.yml test file. It failed first, then it passed when my changes were added.
I removed the restriction set on deletion requiring a name being passed in class/oc_obj.py.

jarrpa · 2017-04-03T19:00:54Z

roles/lib_openshift/src/lib/base.py

@@ -95,11 +95,15 @@ def _create(self, fname):
        '''call oc create on a filename'''
        return self.openshift_cmd(['create', '-f', fname])

-    def _delete(self, resource, rname, selector=None):
+    def _delete(self, resource, name=None, selector=None):
        '''call oc delete on a resource'''
        cmd = ['delete', resource, rname]


Forgot to remove rname.

ashcrow

❤️ the move to format()!

ashcrow · 2017-04-03T19:58:27Z

roles/lib_openshift/library/oc_adm_ca_server_cert.py

        '''call oc delete on a resource'''
        cmd = ['delete', resource, rname]
-        if selector:
-            cmd.append('--selector=%s' % selector)
+        if selector is not None:


ashcrow · 2017-04-03T19:58:42Z

roles/lib_openshift/library/oc_adm_ca_server_cert.py

@@ -941,7 +945,7 @@ def _process(self, template_name, create=False, params=None, template_data=None)
        else:
            cmd.append(template_name)
        if params:
-            param_str = ["%s=%s" % (key, value) for key, value in params.items()]
+            param_str = ["{}={}".format(key, value) for key, value in params.items()]


ashcrow · 2017-04-03T19:59:39Z

roles/lib_openshift/library/oc_adm_ca_server_cert.py

-            cmd.append('--selector=%s' % selector)
-        elif rname:
-            cmd.append(rname)
+        if selector is not None:


Is this a "one or the other" thing as well? If so, should it raise if both are provided? Same question for other similar operations.

@ashcrow, good question. I just tried a query with both given and it is an error when both are supplied.

oc get template mysql --selector=template=mysql-ephemeral-template --all-namespaces error: name cannot be provided when a selector is specified

I will update the module params to be mutually-exclusive.

kwoodson · 2017-04-03T20:24:15Z

@jarrpa, good catch. I pushed an update with rname removed.

jarrpa · 2017-04-03T20:26:16Z

@kwoodson It would be good to replicate some of the changes being made to _delete() in _get(), though that's a different PR. :)

ewolinetz · 2017-04-03T20:43:46Z

roles/lib_openshift/library/oc_obj.py

+        elif name is not None:
+            cmd.append(name)
+        else:
+            raise OpenShiftCLIError('Either name or selector is required when calling delete.')


ashcrow · 2017-04-03T21:01:49Z

************ Module oc_obj
roles/lib_openshift/library/oc_obj.py:1453: [C0330(bad-continuation), ] Wrong continued indentation before block.
           '\"%s\" not found' % self.name in results['stderr']):
           ^|   |
roles/lib_openshift/library/oc_obj.py:1650: [C0326(bad-whitespace), ] Exactly one space required after comma
        mutually_exclusive=[["content", "files"],["selector", "name"]],

kwoodson · 2017-04-04T20:25:19Z

aos-ci-test

kwoodson · 2017-04-04T20:30:18Z

@ewolinetz @ashcrow, I think this is ready now.

ashcrow · 2017-04-04T20:36:47Z

aos-ci-test

ashcrow · 2017-04-04T20:36:49Z

[test]

ewolinetz · 2017-04-04T21:08:19Z

roles/lib_openshift/src/test/integration/oc_obj.yml

+  - name: delete using selector
+    oc_obj:
+      namespace: test
+      selector: name=mysql


does this work as a selector? I would expect that the DC you specify above would have a value here:
https://github.com/openshift/openshift-ansible/pull/3840/files#diff-a1c29b1eb7b762844f88ac765306d442R138

Can we add a list operation right before this delete to verify that we see the object before we delete it as we do on 194?

ewolinetz · 2017-04-04T21:08:46Z

@kwoodson question regarding oc_obj test otherwise LGTM

openshift-bot · 2017-04-04T22:28:24Z

success: aos-ci-jenkins/OS_unit_tests for 54c6226 (logs)

openshift-bot · 2017-04-04T22:57:51Z

success: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" for 54c6226 (logs)

openshift-bot · 2017-04-04T23:00:14Z

success: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" for 54c6226 (logs)

openshift-bot · 2017-04-04T23:02:37Z

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for 54c6226 (logs)

openshift-bot · 2017-04-04T23:05:14Z

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for 54c6226 (logs)

ashcrow · 2017-04-05T13:28:19Z

Looks like another rebase has to occur.

kwoodson · 2017-04-05T19:32:26Z

[merge]

kwoodson · 2017-04-05T19:51:28Z

@ewolinetz @ashcrow would either of you mind 👍 on this?

stevekuznetsov · 2017-04-05T20:07:42Z

@kwoodson you ran the tests a number of times over and over here without changing code -- what was the issue? If there are flakes in the tests, we should all be making issues and tracking the flakes so we can adequately prioritize fixing high-frequency flakes.

kwoodson · 2017-04-05T20:14:41Z

@stevekuznetsov, the tests were failing randomly and intermittently.

No such file or directory
error: aos-ci-jenkins/OS_3.5_containerized for 92f3067 (logs)

I'm not sure why but the tests are the worst part about getting PRs into this repo.

stevekuznetsov · 2017-04-05T20:27:27Z

@kwoodson a test failing intermittently is the textbook definition of a flake. Please log issues for them and help the organization work on eliminating them.

kwoodson · 2017-04-05T20:45:03Z

[merge]

sdodson · 2017-04-05T21:11:03Z

comments above say unit tests were fine, but the status in github is marked failed. I really hate to do this, but lets give this one more shot.

sdodson · 2017-04-05T21:11:10Z

aos-ci-test

openshift-bot · 2017-04-05T21:44:18Z

success: aos-ci-jenkins/OS_unit_tests for 92f3067 (logs)

openshift-bot · 2017-04-05T22:08:06Z

error: aos-ci-jenkins/OS_3.6_containerized for 92f3067 (logs)

openshift-bot · 2017-04-05T22:17:06Z

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for 92f3067 (logs)

openshift-bot · 2017-04-05T22:19:34Z

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for 92f3067 (logs)

openshift-bot · 2017-04-05T22:25:55Z

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for 92f3067 (logs)

stevekuznetsov · 2017-04-05T22:57:13Z

@arilivigni it looks like this PR status for aos-ci-jenkins/OS_unit_tests can't recover from having failed once before

sdodson · 2017-04-06T12:39:36Z

I don't actually see the unit tests having failed at any time in the past week. I made a change to add system container jobs, i'm going to revert that.

sdodson · 2017-04-06T13:01:52Z

aos-ci-test

kwoodson · 2017-04-06T14:23:47Z

flake openshift/origin#10162

kwoodson · 2017-04-06T14:23:59Z

[merge]

openshift-bot · 2017-04-06T14:31:45Z

Evaluated for openshift ansible merge up to 92f3067

openshift-bot · 2017-04-06T15:02:45Z

success: aos-ci-jenkins/OS_unit_tests for 92f3067 (logs)

sdodson · 2017-04-06T15:03:25Z

~~I just pushed the context smashing fix, so that merge is going to fail, aborting it.~~
Nevermind, I'll let the bots race and hope that aos ci test wins.

openshift-bot · 2017-04-06T15:27:28Z

error: aos-ci-jenkins/OS_3.6_containerized for 92f3067 (logs)

openshift-bot · 2017-04-06T15:36:24Z

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for 92f3067 (logs)

openshift-bot · 2017-04-06T15:36:36Z

success: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" for 92f3067 (logs)

openshift-bot · 2017-04-06T15:38:56Z

success: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" for 92f3067 (logs)

openshift-bot · 2017-04-06T16:15:43Z

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/171/) (Base Commit: af85471)

kwoodson changed the title ~~Adding ability to delete by selector. Also variable name cleanup.~~ Adding ability to delete by selector. Also variable name cleanup and update to use .format. Apr 3, 2017

kwoodson requested review from ashcrow and tbielawa April 3, 2017 18:28

kwoodson self-assigned this Apr 3, 2017

kwoodson mentioned this pull request Apr 3, 2017

oc_obj: allow absent/delete with no name #3833

Closed

jarrpa reviewed Apr 3, 2017

View reviewed changes

ashcrow reviewed Apr 3, 2017

View reviewed changes

kwoodson force-pushed the oc_obj_delete_selector branch from 43280d0 to 24a063e Compare April 3, 2017 20:19

kwoodson force-pushed the oc_obj_delete_selector branch from 57543ae to 43e6087 Compare April 3, 2017 20:37

ewolinetz reviewed Apr 3, 2017

View reviewed changes

ewolinetz reviewed Apr 4, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2017

kwoodson force-pushed the oc_obj_delete_selector branch 2 times, most recently from 1965e75 to 4af5b18 Compare April 5, 2017 13:22

Adding delete with selector support.

b7c1adf

ewolinetz approved these changes Apr 5, 2017

View reviewed changes

ashcrow approved these changes Apr 6, 2017

View reviewed changes

sdodson merged commit ff988ba into openshift:master Apr 6, 2017

kwoodson deleted the oc_obj_delete_selector branch September 18, 2017 14:28

Adding ability to delete by selector. Also variable name cleanup and update to use .format. #3840

Adding ability to delete by selector. Also variable name cleanup and update to use .format. #3840

Conversation

kwoodson commented Apr 3, 2017

kwoodson commented Apr 3, 2017

jarrpa Apr 3, 2017

Choose a reason for hiding this comment

ashcrow left a comment

Choose a reason for hiding this comment

ashcrow Apr 3, 2017

Choose a reason for hiding this comment

ashcrow Apr 3, 2017

Choose a reason for hiding this comment

ashcrow Apr 3, 2017

Choose a reason for hiding this comment

kwoodson Apr 3, 2017

Choose a reason for hiding this comment

kwoodson commented Apr 3, 2017

jarrpa commented Apr 3, 2017

ewolinetz Apr 3, 2017

Choose a reason for hiding this comment

ashcrow commented Apr 3, 2017

kwoodson commented Apr 4, 2017

kwoodson commented Apr 4, 2017

ashcrow commented Apr 4, 2017

ashcrow commented Apr 4, 2017

ewolinetz Apr 4, 2017

Choose a reason for hiding this comment

ewolinetz commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

openshift-bot commented Apr 4, 2017

ashcrow commented Apr 5, 2017

kwoodson commented Apr 5, 2017

kwoodson commented Apr 5, 2017

stevekuznetsov commented Apr 5, 2017

kwoodson commented Apr 5, 2017

stevekuznetsov commented Apr 5, 2017

kwoodson commented Apr 5, 2017

sdodson commented Apr 5, 2017

sdodson commented Apr 5, 2017

openshift-bot commented Apr 5, 2017

openshift-bot commented Apr 5, 2017

openshift-bot commented Apr 5, 2017

openshift-bot commented Apr 5, 2017

openshift-bot commented Apr 5, 2017

stevekuznetsov commented Apr 5, 2017

sdodson commented Apr 6, 2017

sdodson commented Apr 6, 2017

kwoodson commented Apr 6, 2017

kwoodson commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

sdodson commented Apr 6, 2017 • edited Loading

openshift-bot commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

openshift-bot commented Apr 6, 2017

sdodson commented Apr 6, 2017 •

edited

Loading