[BUG] state variable can easily collide with other variables

[neilschelly]

Describe the bug
I am getting a collision on the state variable that is defaulted to latest in this cookbook. Specifically, I use the aws_ec2 module for Ansible inventory. This will set a state on every host it discovers representing the instance state.

To Reproduce
Steps to reproduce the behavior:

1. Use the aws_ec2 inventory for hosts.
2. Run the playbook.
3. See error

TASK [artis3n.tailscale : State Validation] **************************************************************************************************************************************
fatal: [tailscale_gateway]: FAILED! => {"changed": false, "msg": "'state' must be 'latest', 'present', or 'absent'."}

Expected behavior
The value for state should be latest, present, or absent. But if the instance was discovered by the aws_ec2 module, state will be something like:

{
    "code": 16,
    "name": "running"
}

Target (please complete the following information):

• OS: N/A
• Ansible version: 2.16.6
• artis3n.tailscale version: 4.5.0
• Tailscale version (set verbose to true): N/A

Additional context
Add any other context about the problem here.

I'm guessing the fix will be to rename the state variable to something like tailscale_state so that it's less likely at least to conflict with other playbooks or modules.

[neilschelly]

Addendum... as a workaround for anyone running into this exact issue, you can add something like the following to your aws_ec2 inventory file(s):

hostvars_prefix: aws_

This will ensure all host variables set by the aws_ec2 inventory module will get prefixed with aws_.

[artis3n]

I've been meaning a breaking change rewrite that would include changing state to artis3n_tailscale_state. But either I get around to doing everything I want in that major release, or I could make this backwards compatible by first checking for artis3n_tailscale_state, then if not present check for state, and if either is present check that the value is one of latest, present, absent or fail upfront validation 🤔

[neilschelly]

I'm a fan of the artis3n_tailscale_state variable, with state only being considered if that is not set (and being ignored if it's not one of the acceptable values for this playbook). There's a bit of logical issue that can come with that though if you set artis3n_tailscale_state to any of the 3 acceptable values by default.

1. Set artis3n_tailscale_state to latest.
2. Playbook never looks at state since the preferred value is already good.
3. Folks who upgrade and have set state to something else will be broken.

In other words, you have to check state first, and then use that variable if (and only if) it's one of the 3 acceptable values for this playbook. Failing that, you can use artis3n_tailscale_state as intended. If state is used, then it would be helpful to print out a deprecation warning so that folks can update their usage of the playbook.

Once the state check is deprecated, you can probably be safe removing the check in the next major release when folks are hopefully going to read the changelog for breaking changes.

[artis3n]

Oh good point. I think I'd instead want to do, if both are defined, to fail if the state values conflict between them. If both are provided and the values match, or if only state is provided, I can print a warning that state is being deprecated and to move to artis3n_tailscale_state.