-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[nuage_master] Adding module calls instead of command for idempotency. #3905
Conversation
I'm unsure how to test these changes as I'm not familiar with nuage. Please test or let me know how so I can test. Thanks. https://trello.com/c/ZPIwuaDr/373-2-idempotency-configure-role-user-permissions |
testing now |
Testing byo/config.yml with |
@vishpat, would you remind reviewing/testing these changes? |
aos-ci-test |
Testing nuage has turned out to be a bit more complicated. I'll try to get this tested tomorrow. |
@sdodson after reviewing the docs for Nuage, I'm not sure where to find RPMs for running the install. Ideas on where I could find those, or is there a contact for someone more familiar with testing this component? |
@mtnbikenc @kwoodson -- @vishpat would know best. I'm fine with leaving this open until they're able to test. |
command: > | ||
{{ openshift.common.client_binary }} adm {{item}} | ||
--config={{ nuage_tmp_conf }} | ||
delegate_to: "{{ nuage_ca_master }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put this at the end of the Configure role/user permissions
so it aligns with the remaining tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move it if it is aesthetically pleasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, just a nit.
namespace: default | ||
resource_name: "{{ item.resource_name }}" | ||
resource_kind: "{{ item.resource_kind }}" | ||
user: "{{ item.user }}" | ||
with_items: "{{nuage_tasks}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/"{{nuage_tasks}}"
/"{{ nuage_tasks }}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this but was not in my original PR.
@@ -23,4 +23,6 @@ nuage_master_crt_dir: /usr/share/nuage-openshift-monitor | |||
nuage_service_account: system:serviceaccount:default:nuage | |||
|
|||
nuage_tasks: | |||
- policy add-cluster-role-to-user cluster-reader {{ nuage_service_account }} | |||
- resource_kind: cluster-role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid value based on [1]:
resource_kind:
description:
- The kind of policy to affect
required: true
default: None
choices: ["role", "cluster-role", "scc"]
aliases: []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good, however, we are unable to test effectively. I think we should move forward with this PR to standardize to oc_ module usage since we've already tested this functionality with other roles.
@sdodson, what do you think about merging this so we don't lose the effort? Do the tests run against nuage? |
No description provided.