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

Add initial support for packer #242

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

rylev
Copy link
Member

@rylev rylev commented Feb 14, 2023

This adds initial support for running packer with our Ansible setup.

Since the way we handle Ansible configuration is slightly unorthodox (i.e., we have the apply script which allows us to handle multiple environments more easily), we have created a wrapper for packer that sets up the Ansible environment properly. Running this wrapper looks like this:

$ ./packer staging docs-rs-builder 

The script works by creating a python virtual environment and copying the proper Ansible configuration into a directory (much in the same way that the apply script does - in fact, I modified the apply script to create the ansible "workspace" through a reuseable function that the new wrapper script uses). It then runs packer build with the correct template.

As a downside, this does require the packer configuration files to be aware of this special environment they're running in and target the correct path for the playbook and inventory file. While I wish this could be more straight forward, this is probably the price we have to pay for supporting multiple environments and running both plain Ansible as well as packer.

@rylev rylev marked this pull request as ready for review February 15, 2023 10:54
@rylev rylev requested review from pietroalbini and jdno February 15, 2023 10:54
@rylev
Copy link
Member Author

rylev commented Feb 15, 2023

Was able to confirm that the image built with packer is able to successfully run the builder when launched as a fresh instance.

Copy link
Member

@jdno jdno left a comment

Choose a reason for hiding this comment

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

Thanks for bootstrapping Packer, @rylev! <3

I think this is a good first step and we can merge it pretty much as-is. In the long-term, I'd like to try to get rid of the apply script and its custom logic to handle the environments. Which seems even more benefitial now that Packer requires these hacks as well. Not sure if we can drop the script yet, though...

packer/docs-rs-builder/builder.pkr.hcl Outdated Show resolved Hide resolved
packer/packer Show resolved Hide resolved
Copy link
Member

@jdno jdno left a comment

Choose a reason for hiding this comment

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

The only blocker for me is the commented out aptitude package, since that is in the common module and shared with other playbooks.

The rest looks good. 👍

@@ -8,7 +8,7 @@
- name: install apt packages
apt:
name:
- aptitude # needed by ansible itself
# - aptitude # needed by ansible itself
Copy link
Member

Choose a reason for hiding this comment

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

This seems like an unrelated change. Was this necessary to build the AMI?

Before running the packer script, you will need to initialize packer:

```bash
packer init ./docs-rs-builder
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be run from within the packer directory, correct? If so, do you want to add a cd packer or so to the snippet?

Comment on lines 10 to 13
data "amazon-parameterstore" "revision" {
name = "/docs-rs/builder/sha"
region = "us-east-1"
}
Copy link
Member

Choose a reason for hiding this comment

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

The formatting is broken here. We run terraform fmt in CI, do we want to do the same for Packer?

BASE_PATH = pathlib.Path(__file__).resolve().parent
VENV_PATH = BASE_PATH / ".venv"

# Ansible changes a lot between releases and deprecates a lot of stuff each of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Ansible changes a lot between releases and deprecates a lot of stuff each of
# Ansible changes a lot between releases and deprecates a lot of stuff in each of

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants