mq-ansible icon indicating copy to clipboard operation
mq-ansible copied to clipboard

Simplify for loop/regex in `Linux_installmq.yml`

Open CharlieParker opened this issue 2 years ago • 5 comments

Discussed early on with @scottcurtis2605, the following Find required package files task is rather confusing:

https://github.com/ibm-messaging/mq-ansible/blob/7243a4a6c20768f8ebd261fc66853f1d00a13087/ansible_collections/ibm/ibmmq/roles/installmq/tasks/Linux_installmq.yml#L18-L60

I'd like to discuss this more, we're fairly sure it can be made cleaner but there might be something we're missing.

Initial Thoughts

  • find files in a certain directory or path /var/MQServer
  • On each iteration the patterns key will have a single value "{{ item }}"
  • What does package_files look like throughout this process? My understanding is it will be rewritten based on the register in every iteration

I think if the intention is to select only files that match a certain regex pattern, then this needs to be rethought.

Luckily it would be simplifying the code/process, I believe find and the following regex should be sufficient:

(?i)(?:.*runtime|gskit|server|java|jre|sdk|samples|man|client|amqp|ams|web|.*(-|_)es|.*(-|_)cn).*

CharlieParker avatar Jul 28 '23 17:07 CharlieParker

The .* at the beginning of the non-capturing group would only work for "runtime" and not the other | separated parts. Placing the .* before the group instead will fix this: (?i).*(?:runtime|gskit|server|java|jre|sdk|samples|man|client|amqp|ams|web|.*(-|_)es|.*(-|_)cn).* This can be optimised to: (?i)^.*?(?:runtime|gskit|server|java|jre|sdk|samples|man|client|amqp|ams|web|[-_](?:es|cn)).*$

J-295 avatar Jul 28 '23 21:07 J-295

Hi @CharlieParker @James-Bennett-295, this is great stuff!

The initial design for this list was to give customers the freedom to select/deselect which components they'd like to install (across both debian and rpm package file patterns) - but you both raise a good point in that a lot of the default components in the list are part of the core MQ installation, where customisation doesn't make sense.

So I'll pass this back to you, do you guys think it would be valuable to have the core installation components as part of one regex pattern as you have proposed, PLUS the additional components (like samples, ams etc.) as single regex patterns for customers to select?

Either way, encapsulating the core mq installation components into one regex pattern would be a welcome optimisation to the codebase!

bimsara-yasitha01 avatar Aug 02 '23 10:08 bimsara-yasitha01

If the intention is to have things like samples and ams (that's this right? 😅) as optional or configurable to the user who simply wants to automate the deployment of mq then I think it shouldn't be done by commenting out regex in an ansible playbook.

I think the options should be exposed perhaps on the cli or even be set in a config file if they're not trivial. I'm imagining the commented out lines are options we don't typically need in a default deployment then?

I think it's perhaps worth us discussing this list and grouping options to make customer selection as intuitive as possible.

CharlieParker avatar Aug 02 '23 12:08 CharlieParker

I would recommend keeping the regex as simple as possible, if that means a couple of extra entries I would prefer that over a complex regex. Customers should be aware of and in control of the packages they are installing, the list makes a suggestion but it is easy to modify in its current form.

martinevansibm avatar Aug 02 '23 12:08 martinevansibm

Potentially useful for further conversation is this snippet that I used to check the logic here:

---
- name: Local test
  hosts: localhost
  # hosts: [amd64]
  tasks:
    - name: What dir are we in?
      ansible.builtin.debug:
        msg: The DIR we're in is {{ playbook_dir }}

    - name: Find required package files
      ansible.builtin.find:
        paths: "{{ playbook_dir }}"
        use_regex: true
        patterns: "{{ item }}"
      register: package_files
      with_items:
        - "*.yml"
        - .*mq.*
        - "*.py"

    - name: Debug package_files from above regex loop
      ansible.builtin.debug:
        var: package_files

I would say, even if you want to stick with the regex implementation for this, I'd break up the logic in the single task Find required package files as there's a few things going on there and it would be more readable broken up a little.

I'd also say we should perhaps define the lists of patterns elsewhere such that we only need to specify an abstracted variable or two (rather than look at a long list of regex patterns)

CharlieParker avatar Aug 03 '23 07:08 CharlieParker