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

IDEMPOTENCY - Are you command/shell grepping? #3335

Closed
tbielawa opened this issue Feb 10, 2017 · 2 comments
Closed

IDEMPOTENCY - Are you command/shell grepping? #3335

tbielawa opened this issue Feb 10, 2017 · 2 comments
Labels
area/idempotency kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/question

Comments

@tbielawa
Copy link
Contributor

Description

We do a lot of 'greps' in the code base. There is no grep module. And lineinfile will change files. This is not cool because running cmd/shell is overkill/hacky for just checking if a string is in a file.

What can you do instead?

Take this snippet from playbooks/common/openshift-cluster/upgrades/post_control_plane.yml for example:

# Check for warnings to be printed at the end of the upgrade:
- name: Check for warnings
  hosts: oo_masters_to_config
  tasks:
  # Check if any masters are using pluginOrderOverride and warn if so, only for 1.3/3.3 and beyond:
  - command: >
      grep pluginOrderOverride {{ openshift.common.config_base }}/master/master-config.yaml
    register: grep_plugin_order_override
    when: openshift.common.version_gte_3_3_or_1_3 | bool
    failed_when: false

  - name: Warn if pluginOrderOverride is in use in master-config.yaml
    debug: msg="WARNING pluginOrderOverride is being deprecated in master-config.yaml, please see https://docs.openshift.com/enterprise/latest/architecture/additional_concepts/admission_controllers.html for more information."
    when: not grep_plugin_order_override | skipped and grep_plugin_order_override.rc == 0

So, what do?

What can you do different? Use the find module instead.

# Check for warnings to be printed at the end of the upgrade:
- name: Check for warnings
  hosts: oo_masters_to_config
  tasks:
  # Check if any masters are using pluginOrderOverride and warn if so, only for 1.3/3.3 and beyond:
  - name : Check if any masters are using pluginOrderOverride and warn if so
    find:
      paths: "{{ openshift.common.config_base }}/master/"
      patterns: "master-config.yaml"
      contains: "pluginOrderOverride"
    register: grep_plugin_order_override
    when:
      - openshift.common.version_gte_3_3_or_1_3 | bool

  - name: Warn if pluginOrderOverride is in use in master-config.yaml
    debug: msg="WARNING pluginOrderOverride is being deprecated in master-config.yaml, please see https://docs.openshift.com/enterprise/latest/architecture/additional_concepts/admission_controllers.html for more information."
    when:
      - not grep_plugin_order_override | skipped
      - grep_plugin_order_override.matched > 0
  • No more failed_when because it's not necessary, if no files are matched the task won't fail, it will just report 0 for results.matched.
  • It will never report changed: true because it only inspects, never modifies. We don't rely on command return codes, we instead check the results of the task to see if there was a match.

Got any thoughts?

@tbielawa tbielawa added area/idempotency kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/question labels Feb 10, 2017
@kwoodson
Copy link
Contributor

kwoodson commented Feb 10, 2017

@tbielawa,

For normal greps, I like your solution. Have to be careful in yaml files when grepping. If the option appears in two levels of the structured data it could potentially be returned as well. This could affect the return results registered.

Since the grep is looking at a yaml file, I think yedit would be a great tool here. It would look like this:
- name: find me something in a yaml file!
yedit:
src: "{{ openshift.common.config_base }}/master/master-config.yaml"
key: pluginOrderOverride
state: list
register: grep_plugin_order_override

So what do you think?

@tbielawa
Copy link
Contributor Author

@kwoodson wait whoa what broh. That's pretty sweet. I set up a test playbook to try this out.

https://gist.github.com/tbielawa/5ebc8a2b76f85385aa8dd664f8783bdd

I really like your post Kenny! We could use these two patterns for reference later. I think they're both great in their respective use-cases (mine for general files, yours for YAML files)

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/idempotency kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/question
Projects
None yet
Development

No branches or pull requests

3 participants