Skip to content
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

Remove TinyPilot Ansible role #1509

Merged
merged 44 commits into from
Jul 21, 2023
Merged

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented Jul 18, 2023

Resolves #1431
Blocked on #1430

As part of our war on Ansible, this PR removes the entire TinyPilot Ansible role. We now only use Ansible to run the uStreamer Ansible role via the Ansible playbook (i.e., install.yml), instead of via the now deprecated TinyPilot Ansible role.

Notes

  1. Moved all the Ansible variables in ansible-role/vars/main.yml to the end of ansible-role-ustreamer/vars/main.yml
  2. Moved the ansible-role/molecule directory to ansible-role-ustreamer/molecule
    • Molecule now directly tests ansible-role-ustreamer
  3. Moved ansible-role/docs/usb-gadget-driver.md to docs/usb-gadget-driver.md (for lack of a better idea)
  4. Deleted what was left of the ansible-role directory
  5. Installed the TinyPilot Debian package directly from the bundler install script
  6. In terms of documentation, I've either removed or reworded any docs that reference the TinyPilot Ansible role (i.e., ansible-role or ansible-role-tinypilot)

You can test this PR on a physical device using the following command:

curl \
  --silent \
  --show-error \
  --location \
  https://raw.githubusercontent.com/tiny-pilot/tinypilot/master/scripts/install-bundle | \
  sudo bash -s -- \
    https://output.circle-artifacts.com/output/job/d575393e-8ff8-4cb8-a596-504bb799c8b2/artifacts/0/bundler/dist/tinypilot-community-20230720T1336Z-1.9.0-64+dde6828.tgz

Review on CodeApprove

@mtlynch
Copy link
Contributor

mtlynch commented Jul 19, 2023

@jdeanwallace - I think apt-get can do .deb package installs directly:

https://superuser.com/a/1244531/55112

@jdeanwallace jdeanwallace marked this pull request as ready for review July 19, 2023 16:13
Copy link
Contributor Author

Automated comment from CodeApprove ➜

@jotaen4tinypilot please review this Pull Request

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

In: Discussion

  1. Moved ansible-role/docs/usb-gadget-driver.md to docs/usb-gadget-driver.md (for lack of a better idea)

👍


In: Discussion
I’ve QA’ed this PR on device, and after installing the 5b3f8bc bundle on a fresh SD card, I only get to see the nginx default welcome page. I double-checked by re-running the process, but that had the same result. I also installed the latest bundle as of master, which appeared fine.

I’m not sure what the problem could be. The contents of the tinypilot Debian package is placed correctly onto the system, and the tinypilot systemd service is up and running. So it looks like the issue is specific to the nginx configuration, but that seems rather odd to me at first glance… 🤔 Just thinking out loud, maybe apt-get install is behaving differently than Ansible’s apt module (which uses aptitude), or it’s something about the order in which the installation is executed now vs. before?

Can you double-check this on your end?


In: bundler/verify-bundle:

> Line 49
  ansible-role-tinypilot

Shall we collapse the array and the for below, and just inline the remaining ansible-role-ustreamer in the if?


👀 @jdeanwallace it's your turn please take a look

Copy link
Contributor Author

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

In: Discussion
Thanks for testing!

It looks like the default NGINX server was still enabled after the TinyPilot Debian package was installed. I was disabling the default NGINX server in the preinst script, but it seems like preinst runs before the nginx dependency is even installed.

I am now disabling the default NGINX server in the postinst script. It seems to be working as expected now.


In: bundler/verify-bundle:

> Line 49
  ansible-role-tinypilot

Done.


👀 @jotaen4tinypilot it's your turn please take a look

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

Approved on CodeApprove
✔️ Approved

🥳 🎉 🎊

Tested on device.


In: Discussion
I can confirm the fix!


👀 @jdeanwallace it's your turn please take a look

@jotaen4tinypilot
Copy link
Contributor

jotaen4tinypilot commented Jul 20, 2023

About the failed “CircleCI Pipeline” job: I was experimenting with one-off bundle builds on CircleCI, and I discovered that we’d need the changes in #1518 to make them work. Unfortunately, the failed job trigger from my experiment seems to persist. It doesn’t mean anything with regards to your PR, though!

Sorry for having abused your branch for this… 🥴 I should have created a separate branch for experimenting right away.

Screenshot 2023-07-20 at 19 51 11

@jdeanwallace jdeanwallace merged commit 34918d7 into master Jul 21, 2023
@jdeanwallace jdeanwallace deleted the drop-ansible-role-tinypilot branch July 21, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete the TinyPilot Ansible role
3 participants