Skip to content
This repository has been archived by the owner on Nov 20, 2023. It is now read-only.

Adds a new role (openshift-label-by-expression) which allows you to apply a label to all objects (except Pods) which match a given expression #340

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

InfoSec812
Copy link
Contributor

@InfoSec812 InfoSec812 commented Nov 21, 2018

What does this PR do?

Adds a new role (openshift-label-by-expression) which allows you to apply a label to all objects (except Pods) which match a given expression.

How should this be manually tested?

Apply the following example playbook:

---
- hosts: localhost

  roles:
    - role: openshift-label-by-expression
      vars:
        target_namespace: labs-ci-cd      ## Apply this to ONLY objects in the specified namespace
        name_pattern: ".*selenium.*"      ## Regex patterns are supported
        label: 'app=selenium'             ## The label to be applied

Is there a relevant Issue open for this?

#339

Other Relevant info, PRs, etc.

N/A

Who would you like to review this?

cc: @oybed @logandonley @tylerauerbeck

Copy link
Contributor

@tylerauerbeck tylerauerbeck left a comment

Choose a reason for hiding this comment

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

@InfoSec812 Is this the right place for this role? Would this be better suited for something like the openshift-applier? I don't want to say one way or the other, but would like to see some other folks thoughts. In my opinion, this seems like something you would want to do because of something app specific, which seems a better fit for the applier.

cc/ @oybed @logandonley for some additional thoughts

@oybed
Copy link
Contributor

oybed commented Nov 22, 2018

@tylerauerbeck yeah, this is the correct place (at least for now). We do not want to have roles co-existing with the openshift-applier, for a few reasons:

  • The roles should not be openshift-applier specific (although their main use may be, they should be able to be re-used elsewhere)
  • In the case of where the openshift-applier uses the roles (and it uses galaxy to pull in the repo), it has been the source for confusion before whereas the openshift-applier uses galaxy on its own repo.

As we are looking to re-structure some of the redhat-cop repos, this can also be part of the discussion, but for now it make sense that openshift specific roles are part of the container automation (a.k.a.: CASL) repo. You will also see that there are other similar roles in this repo - i.e.: openshift-labels, openshift-imagetag, openshift-replicas-ready, openshift-route-status, etc.

@oybed
Copy link
Contributor

oybed commented Nov 22, 2018

@etsauer & @sabre1041 can you please weigh in on the functionality of this implementation / role?

@makentenza
Copy link
Contributor

@oybed @InfoSec812 even I don't really see the reason for having this role here (I'm the same opinion as @tylerauerbeck that this should be in another place, not openshift-applier either) does not make more sense to extend the existing openshift-labels role to include this new capability?

@sabre1041
Copy link
Contributor

@tylerauerbeck @InfoSec812 @oybed A couple thoughts:

#. Agree that it should not be targeted specifically for the openshift-applier, but further discussions will need to occur for all of the roles in casl-ansible, infra-ansible etc on how we plan to release these through galaxy moving forward.

I have serious issues with the logic employed by this role. Specifically:

  • oc get all does not return all objects in a namespace, but only "the most commonly used".
  • The logic for excluding pods is not correct. The name output uses the format pod/*

@oybed
Copy link
Contributor

oybed commented Nov 23, 2018

Yeah, so let's not discuss the move of the role as part of this PR. Fact is that we already have roles in here that serves a similar purpose as this one and it's a logical home for now. Let's discuss the repo structure outside of this, and preferably in one of the chat rooms. For now, this is where the PR will be accepted and let's not hold up progress for Deven based on that topic.

Re: functionality of the role - yes, I agree with @sabre1041. Also, the point @makentenza brings up extending the functionality of the existing labels role was brought up in chat a few days ago. (see the discussion in the day-in-the-life-of-dev chat room around much of the functionality of this role). Maybe it's easier/better to continue the convo in the chat room.

@tylerauerbeck tylerauerbeck dismissed their stale review November 23, 2018 05:03

Dismissing my review since we'll be discussing the location separately from this PR.

@garethahealy
Copy link
Contributor

@oybed ; was anything decided on this PR?

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants