fix: Set __is_system_running in check mode #172
Conversation
Reviewer's GuideThis PR ensures that the Flow diagram for task execution in check modegraph TD
subgraph Task Execution in set_vars.yml
A[Start] --> B{Ansible --check mode?};
B -- No --> E[Run 'systemctl is-system-running'];
B -- Yes --> C{Task has 'check_mode: false'?};
C -- No (Before PR) --> D[Skip command];
C -- Yes (After PR) --> E;
D --> F[Variable '__is_system_running' is not set];
E --> G[Set '__is_system_running' from command output];
F --> H[Later tasks fail];
G --> I[Later tasks succeed];
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The f42 failure is unrelated, it was introduced by #171 (landed red). However, that feels easy to fix, I'll have a quick look. Running citest in the meantime to check centos 8. |
|
[citest] |
|
OK, so centos 8 is happy, and I fixed the types of the assertions. This was actually hiding a bug! It now fails like this: So the check expectes "scope=all", but reality is "scope=none". So |
martinpitt
force-pushed
the
check-mode-running
branch
2 times, most recently
from
e707549 to
d225ee7
Compare
martinpitt
force-pushed
the
check-mode-running
branch
from
d225ee7 to
5a1be2d
Compare
|
Now it should be back to failing on the scope=all/none mismatch. I didn't have time to really look into the code today, just having 3 min looks in between the sessions. @richm if you want to work on this, please feel free -- otherwise I'll have a look on the train ride back on Sunday. |
@justin-stephenson any ideas? |
Sorry I don't quite follow, what is the issue? By the way, below we should change the name of I can do it in a separate PR after this one is merged. |
|
@richm oh, I know what's wrong -- tests/tests_sssd.yml needs to define |
'So the check expectes "scope=all", but reality is "scope=none".' but it looks like @martinpitt identified the problem: #172 (comment) 'oh, I know what's wrong -- tests/tests_sssd.yml needs to define tlog_scope_sssd and other vars for both the role invocation and importing run_sssd_tests.yml' and he is going to fix it.
ok - thanks |
|
With the [WARNING]: conditional statements should not include jinja2 templating delimiters such as {{ }} or {% %}. Found: 'scope={{ So I rewrote that part again. I also fixed the sssd config test failure from above. However, with 2.19 there is now yet another failure: This is really hard to understand, and feels like an Ansible bug (or it's just getting late and we had beer 😁 ). But at least the other OSes should go green. |
martinpitt
force-pushed
the
check-mode-running
branch
from
5a1be2d to
2517034
Compare
|
[citest] |
martinpitt
force-pushed
the
check-mode-running
branch
from
2517034 to
00fd78d
Compare
|
[citest] |
|
@richm Still red due to the new (later) 2.19 failure. But this improves things, and I think this could go in already. Bedtime now. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @martinpitt - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Wrt. the final ansible 2.19 failure: when I do this: it fails with So something changed or broke with the template reading. |
|
|
tests/tasks/check_header.yml will need to do something like this: - name: Check for presence of ansible managed header, fingerprint
assert:
that:
- __ansible_managed in content
- __fingerprint in content
vars:
content: "{{ (__file_content | d(__content)).content | b64decode }}"
__ansible_managed: "{{ lookup('template', 'get_ansible_managed.j2') }}" |
|
looks like ansible 2.19 was changed to make |
Cause: The test used a temporary variable `ansible_managed`, but that is a "magic" string constant. Ansible 2.19 does not permit assigning to it any more. Consequence: Tests failed with Ansible 2.19. Fix: Rename the variable.
Cause: `systemctl is-system-running` was not called in `--check` mode. Consequence: The "Determine if system is booted with systemd" step failed in check mode as the `__is_system_running` variable was not populated. Fix: Force calling `systemctl is-system-running` in check mode. It does not modify the system and the outcome is very influential for what the role does.
Cause: Ansible 2.19 requires explicit boolean types for assertions.
Consequence: The test failed with Ansible 2.19 with "Conditional result
was '62' of type 'int', which evaluates to True. Conditionals must have
a boolean result."
Fix: find() does not actually have a result which is compatible with
boolean coercion, as 0 means "string found at the start", and "not
found" results in -1 (which is `True`). We could compare the find()
result explicitly, but let's instead use the `in` operator to make this
more readable.
Also rewrite these assertions to use Python string concatenation. This
fixes
> [WARNING]: conditional statements should not include jinja2 templating delimiters such as {{ }} or {% %}. Found: 'scope={{
tlog_scope_sssd }}' in __tlog_conf_content.stdout
Likewise, `regex_search()` returns a string (possibly empty for
non-matches) or `none`, so explicitly check the length to convert to a
bool.
Group the test cases in blocks which use the same definition of `tlog_scope_sssd` and other variables for both the role invocation and check_sssd_with_tlog.yml. Then the latter can actually check for the correct value. This was previously masked by the wrong `find()` result evaluation.
martinpitt
force-pushed
the
check-mode-running
branch
from
00fd78d to
a4b8796
Compare
|
Thanks @richm that saved me some research this morning. This works fine! |
|
[citest] |
martinpitt
requested review from
richm and
spetrosi
and removed request for
richm
|
Whew, all green 🍀 I suppose these issues apply to more packages, so at least now we have a nice reference fix. |
| vars: | ||
| content: "{{ (__file_content | d(__content)).content | b64decode }}" | ||
| ansible_managed: "{{ lookup('template', 'get_ansible_managed.j2') }}" | ||
| content: "{{ __content.content | b64decode }}" |
There was a problem hiding this comment.
yeah - I copied this implementation from another role where I already have the file content, so I could skip the slurp - but here it isn't used
4 participants
Cause:
systemctl is-system-runningwas not called in--checkmode.Consequence: The "Determine if system is booted with systemd" step failed in check mode as the
__is_system_runningvariable was not populated.Fix: Force calling
systemctl is-system-runningin check mode. It does not modify the system and the outcome is very influential for what the role does.See failure in #109 log and #170 (comment) ff. I cannot reproduce this locally, as
fails in a completely different way. So throwing this at CI.
Summary by Sourcery
Force execution of
systemctl is-system-runningin check mode and update SSSD test playbooks to align assertions and structure with this behavior.Bug Fixes:
systemctl is-system-runningwith check_mode disabled to populate the__is_system_runningvariable.Tests:
run_sssd_tests.ymlassertions to useinoperators with helper string variables for list-based parameters.check_sssd_with_tlog.ymlassertion to explicitly check for regex match length.