Workflow and testing improvements #328
Conversation
| - handlers_tailscale_status.stdout | length != 0 | ||
| - handlers_tailscale_status.stdout is not match('\[L\+V9o\]') | ||
| - tailscale_is_online |
There was a problem hiding this comment.
I'm actually not sure what exactly this condition was trying to assert, but I think the new condition reflects better the name of the task.
There was a problem hiding this comment.
Tailscale's output had some mysterious behavior in the past. I think --json solves for it, though
#43 (comment)
There was a problem hiding this comment.
😄 When I saw that regexp, I was sure there was some amount of debugging time spent right there.
|
Love the idea of working on the JSON instead. As for
I remember having to dig through tailscale source to find out how exit codes were produced by the binary, and the resulting reason led to this setup. It's been a couple years since then, though. The binary has changed a lot and my cursory dive back into the code couldn't find the same location I remember. Worth rethinking how that works as well, but I'll want to test this for a little while before moving forward. |
| - handlers_tailscale_status.stdout | length != 0 | ||
| - handlers_tailscale_status.stdout is not match('\[L\+V9o\]') | ||
| - tailscale_is_online |
There was a problem hiding this comment.
Tailscale's output had some mysterious behavior in the past. I think --json solves for it, though
#43 (comment)
If that makes things a bit easier, the dropped failed_when:
- tailscale_status.rc != 0
- "'Logged out.' not in tailscale_status.stdout"
- "'not logged in' not in tailscale_status.stdout"to default behavior: failed_when:
- tailscale_status.rc != 0In this case the other two conditions were likely to account for unexpected output. In this new implementation we would notice that when parsing json. The |
mprasil
force-pushed
the
workflow-improvements
branch
from
f716fc2 to
81d1755
Compare
|
I have now dropped the separate |
artis3n
force-pushed
the
workflow-improvements
branch
from
81d1755 to
29c9119
Compare
This patch redacts the authkey from `tailscale up` stderr output based on `tailscale_authkey` directly instead of the `tskey.*` regex. The same change was already applied for the stdout logging in artis3n#328. diff --git a/tasks/install.yml b/tasks/install.yml index 4719173..a9db923 100644 --- a/tasks/install.yml +++ b/tasks/install.yml @@ -167,6 +167,6 @@ - name: Install | Report redacted failure from "tailscale up" # noqa: no-handler ansible.builtin.fail: - msg: "{{ tailscale_start.stderr | default () | regex_replace('tskey.*\\s', 'REDACTED ') | regex_replace('\\t', '') | split('\n') }}" + msg: "{{ tailscale_start.stderr | default () | regex_replace(tailscale_authkey, 'REDACTED') | regex_replace('\\t', '') | split('\n') }}" when: - tailscale_start is failed
This patch redacts the authkey from `tailscale up` stderr output based on `tailscale_authkey` directly instead of the `tskey.*` regex. The same change was already applied for the stdout logging in artis3n#328.
This patch redacts the authkey from `tailscale up` stderr output based on `tailscale_authkey` directly instead of the `tskey.*` regex. The same change was already applied for the stdout logging in #328.
This patch redacts the authkey from `tailscale up` stderr output based on `tailscale_authkey` directly instead of the `tskey.*` regex. The same change was already applied for the stdout logging in artis3n#328.
This is somewhat opinionated PR with the intention to improve the role workflow a bit, make it a bit more readable and less fragile. I'm very open to discuss individual changes. I have split it into two commits that are effectively unrelated, it's just I encountered the test bug while working on the rest.
05407c2
Adds extra steps to update hosts file in test container. Tailscale does attempt to manage
resolv.confwhich is somewhat in conflict with Docker attempting to provice container name resolution. This is problematic in multi-stage scenarios where tailscale is initialized multiple times and eventually ends up in a state where container name resolution no longer works and test fails.This extra steps add static hosts override for headscale container so that Tailscale managing DNS resolution does not mess up the connectivity to Headscale container.
f716fc2
The main changes are:
Switch to parsing json output of
tailscale status- this should hopefully lead to less fragile detection of current status of tailscale connection and should also end up with more readable conditional logic in the role.Split
tailscale upstep into two separate steps depending on situation:tailscale upwithout any arguments is used as the connection should already be properly set and this is all that is needed to bring the connection up.tailscale upwith all the arguments is executed.This can enable use cases where we don't necessarily want to fully manage the tailscale connection via Ansible, but the user still wants to bring the connection up. (ie. to prepare the instance for subsequent roles that rely on that connections)
failed_whenandchanged_whenconditions and instead rely on default behavior, that assumes non-zero return is failure and that any time command is executed (excepttailscale status) it is performing a change. This probably reflects reality better.