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

oc_obj: Allow for multiple kinds in delete #3968

Merged
merged 1 commit into from
Apr 25, 2017

Conversation

jarrpa
Copy link
Contributor

@jarrpa jarrpa commented Apr 21, 2017

This is to allow "kind" to include a string like "svc,ep" to delete both Services and Endpoints matching a given name or selector.

Signed-off-by: Jose A. Rivera [email protected]

@kwoodson
Copy link
Contributor

@jarrpa, @ashcrow, would it make sense to make the kind a list so the interface is a bit cleaner?

kind: 
- ep
- svc

I think this makes the api a bit cleaner. Thoughts?

@jarrpa
Copy link
Contributor Author

jarrpa commented Apr 21, 2017

@kwoodson @ashcrow Maybe. I just went with the svc,ep format because that's what the command line format is. It'd be fairly trivial to do something like self.kind.join(','), but I'm not sure what implications that would have on the rest of oc_obj. As the comma-separated format works with everything else as-is, maybe consider a cleanup of the API as a near future PR (this PR would also include the required change to the GlusterFS code)? I could probably take that on (since this is my fauilt ;) ), I just foresee a lot of careful review being required and would rather that not hold up my current work.

@ashcrow
Copy link
Member

ashcrow commented Apr 21, 2017

I think a list would be cleaner, but I think a '[ ]?,' splittable string is good enough for now.

@tbielawa
Copy link
Contributor

I think the comma-separated string format could be OK. That's how the command line expects to take arguments, right? So it's consistent with that.

But, this is a computer program, and a user might expect to provide a series of items as a list (data structure). So it might not be consistent with what users expect.

I'm good with either way. Obviously if it changes to accepting a list instead of a comma separated string more changes will be required.

If we stick with the comma separated string then I would like to see roles/lib_openshift/src/doc/obj updated with a note indicating a comma separated list of values is also acceptable input.

@jarrpa
Copy link
Contributor Author

jarrpa commented Apr 21, 2017

Updated the documentation as requested by @tbielawa.

@ashcrow
Copy link
Member

ashcrow commented Apr 21, 2017

Checking generated module code: roles/lib_openshift/src/generate.py
('Generated content does not match for /home/travis/build/openshift/openshift-ansible/roles/lib_openshift/src/../library/oc_obj.py',)
Checking generated module code: roles/lib_utils/src/generate.py

@jarrpa
Copy link
Contributor Author

jarrpa commented Apr 21, 2017

@ashcrow Yup, just caught that. :) Addressed and updated.

# verify results are empty for the selector
if params['selector'] is not None and len(api_rval['results']) == 0:
# verify its not in our results
if (params['name'] is not None or params['selector'] is not None) and \
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually prefer the logic in 2 separate steps as its more clear to me what's happening.

I won't block on this.

@kwoodson
Copy link
Contributor

aos-ci-test

@openshift-bot
Copy link

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

@openshift-bot
Copy link

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

@openshift-bot
Copy link

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

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for 914200d (logs)

@kwoodson
Copy link
Contributor

[merge]

@openshift-bot
Copy link

Evaluated for openshift ansible merge up to 914200d

@openshift-bot
Copy link

openshift-bot commented Apr 25, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/285/) (Base Commit: d4d1437)

@openshift-bot openshift-bot merged commit d5ec349 into openshift:master Apr 25, 2017
@jarrpa jarrpa deleted the oc_obj-kinds branch June 7, 2017 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants