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

fix: make enabling/disabling non-existent services not fail in check mode #153

Merged
merged 14 commits into from
Jul 12, 2023

Conversation

BrennanPaciorek
Copy link
Collaborator

Enhancement:

  • firewall_lib.py - check if service exists before running firewalld methods that would cause failure
    • fails if service does not exist and in diff mode, warns if in check mode and service does not exist
  • README.md - reflects changes and explains interaction with check mode
  • tests/tests_service.yml - add integration test case for adding non-existent services in check mode
  • tests/unit/test_firewall_lib.py - Mock necessary output from fw.config().getServiceNames()

Reason:
Better compliance with Ansible best practices for check mode (not failing in check mode, especially where they would not fail in diff mode)
Reason for this particular solution - We cannot track changes from previous check modes without overhauling how check mode is handled throughout the entire system role.

Result:
Undefined services being enabled or disabled will not result in failure while in check mode, but a warning will be displayed intended to prompt the user to confirm that the service is defined in a previous play, since the same action could result in failure when run in diff mode.

Issue Tracker Tickets (Jira or BZ if any):

- Addresses linux-system-roles#146
- firewall_lib.py - check if service exists before running firewalld methods that would cause failure
  - fails if service does not exist and in diff mode, warns if in check mode and service does not exist
- README.md - reflects changes and explains interaction with check mode
- tests/tests_service.yml - add integration test case for adding non-existent services in check mode
- tests/unit/test_firewall_lib.py - Mock necessary output from fw.config().getServiceNames()
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.23 🎉

Comparison is base (f458cb4) 53.39% compared to head (11bc6ba) 53.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
+ Coverage   53.39%   53.62%   +0.23%     
==========================================
  Files           2        2              
  Lines         796      800       +4     
==========================================
+ Hits          425      429       +4     
  Misses        371      371              
Flag Coverage Δ
sanity ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
library/firewall_lib.py 63.08% <100.00%> (+0.21%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BrennanPaciorek BrennanPaciorek changed the title make enabling/disabling non-existent services not fail in check mode fix: make enabling/disabling non-existent services not fail in check mode Jul 11, 2023
@richm
Copy link
Contributor

richm commented Jul 11, 2023

ansible-lint: add var-naming[no-role-prefix] to skip_list in .ansible-lint

tests/tests_service.yml Outdated Show resolved Hide resolved
@richm
Copy link
Contributor

richm commented Jul 11, 2023

[citest]

BrennanPaciorek and others added 2 commits July 11, 2023 17:38
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
tests/tests_service.yml Outdated Show resolved Hide resolved
tests/tests_service.yml Outdated Show resolved Hide resolved
BrennanPaciorek and others added 2 commits July 11, 2023 17:50
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
tests/tests_service.yml Outdated Show resolved Hide resolved
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
tests/tests_service.yml Outdated Show resolved Hide resolved
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
tests/tests_service.yml Outdated Show resolved Hide resolved
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
@richm
Copy link
Contributor

richm commented Jul 11, 2023

You might find this useful - https://linux-system-roles.github.io/documentation/howto/working-with-ansible-jinja-code-and-filters.html - "How to solve some common ansible-lint issues"

@richm
Copy link
Contributor

richm commented Jul 11, 2023

[citest]

@spetrosi
Copy link
Contributor

ansible-lint: add var-naming[no-role-prefix] to skip_list in .ansible-lint

Should we add this in .github for all roles or am I missing something?

@richm
Copy link
Contributor

richm commented Jul 12, 2023

ansible-lint: add var-naming[no-role-prefix] to skip_list in .ansible-lint

Should we add this in .github for all roles or am I missing something?

yes, we should - I don't see any other way around this, unfortunately

@richm richm merged commit 156614e into linux-system-roles:main Jul 12, 2023
17 checks passed
@Alveel
Copy link

Alveel commented Jul 20, 2023

Great work, thank you so much!

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.

4 participants