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

Skip installing Ansible as part of bundle creation #1500

Merged
merged 6 commits into from
Jul 12, 2023
Merged

Conversation

mtlynch
Copy link
Contributor

@mtlynch mtlynch commented Jul 12, 2023

We used to use Ansible to download external Ansible roles. As of #1469, we no longer have external Ansible roles, so we can skip installing Ansible as part of creating the TinyPilot install bundle.

I don't think we need to test manually, as the bundles are identical modulo timestamps (which I removed for this comparison):

https://gist.github.com/mtlynch/c65d8ad745f750cc6dd1b8ac93955650

This change cuts about 90s from the bundle build workflow.

Before

image

After

image

Review on CodeApprove

We used to use Ansible to download external Ansible roles. As of #1469, we no longer have external Ansible roles, so we can skip installing Ansible as part of creating the TinyPilot install bundle.
Copy link
Contributor Author

mtlynch commented Jul 12, 2023

Automated comment from CodeApprove ➜

@jdeanwallace please review this Pull Request

Copy link
Contributor

@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 ➜

⏳ Approval Pending (1 unresolved comments)
Approval will be granted automatically when all comments are resolved

Oh nice catch, thanks! LGTM.


In: bundler/create-bundle:

> Line 78
mkdir "${ANSIBLE_ROLES_DIR}"

I think we'll still need to clear out any previous Ansible role directories (i.e., roles/ansible-role-tinypilot and roles/ansible-role-ustreamer) because if we run create-bundle multiple times (as might be the case when using the install-from-source script) then the previous Ansible role directories would clash with the current directories when we copy them in below.


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

Copy link
Contributor Author

@mtlynch mtlynch 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: bundler/create-bundle:

> Line 78
mkdir "${ANSIBLE_ROLES_DIR}"

Ah, right! Fixed.

Copy link
Contributor

@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 ➜

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

@mtlynch mtlynch enabled auto-merge (squash) July 12, 2023 19:25
@mtlynch mtlynch merged commit 07e43d2 into master Jul 12, 2023
@mtlynch mtlynch deleted the skip-bundle-ansible branch July 12, 2023 19:26
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.

2 participants