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

sap_general_preconfigure: Use tags for limiting the role scope #653

Merged
merged 9 commits into from
Feb 28, 2024

Conversation

berndfinger
Copy link
Member

@berndfinger berndfinger commented Feb 12, 2024

Please see the updates of the README.md file for examples for using tags with this role (currently only for RHEL systems).
Solves #342 but leaves the related role parameters (e.g. sap_general_preconfigure_configuration, sap_general_preconfigure_2772999_02) in place and functional because no deprecation note has been published yet.

... for limiting the role scope.

Relates to sap-linuxlab#342.

Sample playbook calls:

$ ansible-playbook sap.yml --tags \
sap_general_preconfigure_configuration,sap_general_preconfigure_configuration_all_steps

$ ansible-playbook sap.yml --tags \
sap_general_preconfigure_configuration,sap_general_preconfigure_2772999_02

$ ansible-playbook sap.yml --tags \
sap_general_preconfigure_configuration,sap_general_preconfigure_selinux

$ ansible-playbook sap.yml --tags \
sap_general_preconfigure_configuration,sap_general_preconfigure_configuration_all_steps \
--skip-tags sap_general_preconfigure_2772999_02

$ ansible-playbook sap.yml --tags \
sap_general_preconfigure_configuration,sap_general_preconfigure_configuration_all_steps \
--skip-tags sap_general_preconfigure_selinux

Signed-off-by: Bernd Finger <[email protected]>
with_items: "{{ __sap_general_preconfigure_sapnotes_versions | difference(['']) }}"
loop_control:
loop_var: sap_note_line_item
tags:
- sap_general_preconfigure_configuration
- sap_general_preconfigure_configuration_all_steps
Copy link
Member

Choose a reason for hiding this comment

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

Is the tag sap_general_preconfigure_configuration_all_steps really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the tag sap_general_preconfigure_configuration_all_steps really necessary?

Thanks! I tested again and it found no longer a combination of tags which would require both tags. So it seems it's really not required. I'll update the code accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was a bit too fast. Further testing revealed that only with both tags, sap_general_preconfigure_configuration and sap_general_preconfigure_configuration_all_steps, it is possible to execute just one or a few of the configuration steps.
With just the tag sap_general_preconfigure_configuration, it is possible to run all configuration steps and skip some of them. But I want more. ;-)
See also the examples in README.md.

ansible.builtin.include_tasks:
file: '{{ item }}/{{ __sap_general_preconfigure_fact_assert_filename_prefix }}installation.yml'
apply:
tags: sap_general_preconfigure_installation
Copy link
Member

Choose a reason for hiding this comment

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

There is already a tag in this task, why adding the same one?

Copy link
Member Author

@berndfinger berndfinger Feb 14, 2024

Choose a reason for hiding this comment

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

This code will apply a tag to all tasks of the included file(s). And without the tags: lines at the end of the same task, this current task will not be run when using that tag. See also: https://docs.ansible.com/ansible/latest/collections/ansible/builtin/include_tasks_module.html .

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

ansible.builtin.include_tasks:
file: '{{ item }}/{{ __sap_general_preconfigure_fact_assert_filename_prefix }}configuration.yml'
apply:
tags: sap_general_preconfigure_configuration_all_steps
Copy link
Member

Choose a reason for hiding this comment

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

Same question done in the line 102

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

@sean-freeman sean-freeman removed their request for review February 14, 2024 18:39
Copy link
Member

@rhmk rhmk left a comment

Choose a reason for hiding this comment

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

The changes look good to me and really improve usability

ansible.builtin.include_tasks:
file: '{{ item }}/{{ __sap_general_preconfigure_fact_assert_filename_prefix }}installation.yml'
apply:
tags: sap_general_preconfigure_installation
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Wabri Wabri left a comment

Choose a reason for hiding this comment

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

LGTM

@berndfinger
Copy link
Member Author

After discussing an alternative solution with a colleague (for avoiding two parameters when running all or most configuration steps), I will test another approach. Moving to draft now.

@berndfinger berndfinger marked this pull request as draft February 15, 2024 15:32
…ion steps

Examples for using tags: See README.md.

Relates to: sap-linuxlab#342.

Signed-off-by: Bernd Finger <[email protected]>
@berndfinger berndfinger marked this pull request as ready for review February 16, 2024 17:06
@berndfinger
Copy link
Member Author

This version is now easier to use and eliminates the need for specifying two tags when running all configuration steps. The only little disadvantage is that now, when using the tag sap_general_preconfigure_configuration only, also some header lines of configuration tasks (displaying the SAP note numbers and versions) are displayed. But as using the tags should not be a default, I think this can be accepted.
See the additional lines in README.md for examples. Thanks to @Wabri and @ja9fuchs for triggering this change!

@berndfinger
Copy link
Member Author

BTW We now know that #652 (ansible-test-sanity for every PR) works as expected. Run time was 1m 32s. Thanks @Wabri !

@rainerleber
Copy link
Contributor

LGTM. This PR is a really cool improvement.

Copy link
Contributor

@ja9fuchs ja9fuchs left a comment

Choose a reason for hiding this comment

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

Great stuff. 👍

@berndfinger berndfinger merged commit 7634e2e into sap-linuxlab:dev Feb 28, 2024
3 of 4 checks passed
@berndfinger berndfinger deleted the issue-342 branch February 28, 2024 10:22
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