tinypilot icon indicating copy to clipboard operation
tinypilot copied to clipboard

Upgrade to Ansible 2.10.7.

Open jdeanwallace opened this issue 2 years ago • 1 comments

Part of https://github.com/tiny-pilot/ansible-role-tinypilot-pro/issues/119 Dependent on https://github.com/tiny-pilot/ansible-role-tinypilot/pull/213

This PR upgrades Ansible from 2.9.13 to 2.10.7. For context, please review https://github.com/tiny-pilot/ansible-role-tinypilot/pull/213 before reviewing this PR.

This PR has been successfully built here.

Notes

  1. There seemed to be a behavioural change from Ansible 2.9 to 2.10. The ansible_user parameter no longer gets automatically defined during a local Ansible connection. As seen by this failed build (after upgrading Ansible):

    The task includes an option with an undefined variable. The error was: 'ansible_user' is undefined

    We felt this issue before when we had to set ansible_user: root for our molecule tests.

    ansible_user is a strange variable because it can either inherit its value from remote_user (i.e. ssh user) or be explicitly defined. However, if remote_user is not set (as is with a local connection) then ansible_user will also be undefined. This is vaguely described in the Ansible docs, but confirmed during local testing.

    I'm not sure why this ever worked for us in the past, but now in Ansible 2.10 ansible_user is always undefined because we use a local connection and we never specify a user when running ansible-playbook.

    We can fix the issue in 2 ways:

    1. (implemented here) Explicitly set a remote_user via the ansible-playbook --user "${USER}" argument, or
    2. Statically set remote_user = root in our ansible.cfg file. This would also work for us because our bundle install script requires sudo privileges so the user will always be root.

Review on CodeApprove

jdeanwallace avatar Aug 11 '22 13:08 jdeanwallace

Automated comment from CodeApprove ➜

⏳ @mtlynch please review this Pull Request

jdeanwallace avatar Aug 12 '22 13:08 jdeanwallace

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

This CodeApprove comment is misleading because yes all your comments have been resolved, but I just added an unresolved comment. So I don't agree that the above comment should be all "green and good to go".

Screen Shot 2022-08-15 at 15 08 54

jdeanwallace avatar Aug 15 '22 13:08 jdeanwallace

This CodeApprove comment is misleading because yes all your comments have been resolved, but I just added an unresolved comment. So I don't agree that the above comment should be all "green and good to go".

Oh, I'm not sure what correct behavior would be in this case, since you did address all my comments. Should we file a bug on CodeApprove requesting different behavior?

mtlynch avatar Aug 15 '22 13:08 mtlynch

Oh, I'm not sure what correct behavior would be in this case, since you did address all my comments. Should we file a bug on CodeApprove requesting different behavior?

Yeah, the current behavior feels misleading to me, but it's just a personal preference. I'll file an issue on their GitHub and let them decide.

jdeanwallace avatar Aug 15 '22 13:08 jdeanwallace