-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
[FEAT] Improve tags support #407
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR! Some documentation and code changes requested, and some debugging needed on the tag:
prepend. You can test locally by following these instructions and running the various make
commands in the Makefile. I recommend make test-default
for the "default" molecule scenario, and make test-oauth
for the oauth test.
README.md
Outdated
> **If authenticating with an OAuth key, do not use this for `--advertise-tags`.** | ||
> Use the `tailscale_oauth_tags` variable instead. | ||
> `--advertise-tags` may be used in this variable if you are not authenticating to Tailscale with OAuth. | ||
> Use the `tailscale_tags` variable instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace this entirely with:
> **Do not use this for `--advertise-tags`.**
> Use the `tailscale_tags` variable instead.
tasks/install.yml
Outdated
no_log: "{{ not (insecurely_log_authkey | bool) }}" | ||
|
||
- name: Install | Authkey Type | ||
ansible.builtin.debug: | ||
msg: "{{ tailscale_authkey_type | trim }}" | ||
when: verbose | ||
|
||
- name: Install | Build the final tailscale_args | ||
ansible.builtin.set_fact: | ||
tailscale_args: "{{ tailscale_args }} {{ tailscale_tags_string | trim }} --timeout={{ tailscale_timeout | trim }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are binding --timeout
, that needs to be added to the list of flags that are not acceptable to input into tailscale_args
on the README.
Co-authored-by: Ari Kalfus <[email protected]>
I appreciate detailed reviewing and comments. docker.errors.DockerException: Error while fetching server API version: ('Connection aborted.', FileNotFoundError(2, 'No such file or directory'))
make: *** [test-default] Error 1 But I added debug tasks and tested the installation on a real machine: TASK [ansible-role-tailscale : Install | Tailscale version and online status] ****************************************************************
ok: [servername] =>
msg: 'Ver: 1.56.1-t906f85d10-g34ed54c8c Online: False'
TASK [ansible-role-tailscale : Install | Prepend 'tag:' to each item in the list] ************************************************************
ok: [servername] => changed=false
ansible_facts:
tailscale_prepared_tags:
- tag:testnode
- tag:devenv
- tag:ci-worker
TASK [ansible-role-tailscale : Install | Build `tailscale up` arguments strings] *************************************************************
ok: [servername] => changed=false
censored: 'the output has been hidden due to the fact that ''no_log: true'' was specified for this result'
TASK [ansible-role-tailscale : Install | Authkey Type] ***************************************************************************************
ok: [servername] =>
msg: OAuth Client Secret
TASK [ansible-role-tailscale : Install | Build the final tailscale_args] *********************************************************************
ok: [servername] => changed=false
ansible_facts:
tailscale_args_string: ' --advertise-tags=tag:testnode,tag:devenv,tag:ci-worker --timeout=120s'
TASK [ansible-role-tailscale : Install | Final `tailscale up` arguments string] **************************************************************
ok: [servername] =>
msg: --advertise-tags=tag:testnode,tag:devenv,tag:ci-worker --timeout=120s
|
I will set up a Linux environment for molecule, but later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Thanks for these contributions
* better tags handling * better tags handling * fix * Apply suggestions from code review Co-authored-by: Ari Kalfus <[email protected]> * fix review comments * tailscale_up_timeout to README --------- Co-authored-by: Ari Kalfus <[email protected]>
🎄 🎅 🌲 Merry Christmas PR that fixes #406
Besides #406, the PR adds default 120s timeout for
tailscale up
.tailscale up
can hang forever in the background if--timeout
is not set.