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 macOS support; input validation; an example #13

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ build_dir=build/pkg
dist_dir=build/dist

# Variables that are replaced in build time
bin_path=/usr/bin
lib_path=/usr/lib/ansible-bundler
etc_path=/etc/ansible-bundler
bin_path=.
lib_path=../lib/ansible-bundler
etc_path=../../etc/ansible-bundler
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to change any of that, even if you just want to run bin/bundle-playbook, you don't have to build it.

Changing these will prevent the build for other platforms.

Copy link
Author

@l50 l50 Apr 30, 2023

Choose a reason for hiding this comment

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

Thank you for your feedback. I understand that changing these variables might prevent the build for other platforms. My intention was to avoid hardcoding the paths, as it could cause issues in different environments. However, I understand that it might not be the best solution for all cases.

Would you have any suggestions on how to make these paths more flexible without affecting other platforms? I am open to any ideas that could help us find a more suitable solution.

version=$(shell cat VERSION)

# Package variables
Expand All @@ -32,7 +32,13 @@ package:
cp -r app/bin $(build_dir)/usr
cp -r app/etc $(build_dir)/etc/ansible-bundler
cp -r app/lib $(build_dir)/usr/lib/ansible-bundler
sed -i'' \
UNAME=$$(uname); \
if [ "$$UNAME" == 'Darwin' ]; then \
SED_BINARY=$$(which gsed); \
else \
SED_BINARY=$$(which sed); \
fi; \
$$SED_BINARY -i'' \
l50 marked this conversation as resolved.
Show resolved Hide resolved
-e 's#LIB_PATH=.*#LIB_PATH=$(lib_path)#' \
-e 's#ETC_PATH=.*#ETC_PATH=$(etc_path)#' \
-e 's#VERSION=.*#VERSION=$(version)#' \
Expand Down
16 changes: 16 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,19 @@ Please make sure to update tests as appropriate. For more information, please re

This project is licensed under the BSD 3-Clause License - see the [LICENSE.md](LICENSE.md) file for
details.

## Example
l50 marked this conversation as resolved.
Show resolved Hide resolved

Before reading this, be sure to follow the instructions
above to build the package.

```shell
# Starting from the root of this repository, run this command:
cd build/pkg/usr/bin

# Employ the basic playbook example
./bundle-playbook -f ../../../../examples/basic.yaml

# Run the playbook
../../../../examples/basic.run -e example=VALUE
```
38 changes: 26 additions & 12 deletions app/bin/bundle-playbook
Original file line number Diff line number Diff line change
Expand Up @@ -194,20 +194,20 @@ copy_playbook() {
shift 4; local extra_deps=("${@}")

echo "Adding playbook..."
cp -L --preserve=timestamps "$playbook_file" "$tmpdir/playbook.yml"
cp -L -p "$playbook_file" "$tmpdir/playbook.yml"
l50 marked this conversation as resolved.
Show resolved Hide resolved
if [ -d "$playbook_path/roles" ]; then
echo "Adding roles..."
cp -Lr --preserve=timestamps "$playbook_path/roles" "$tmpdir"
cp -Lrp "$playbook_path/roles" "$tmpdir"
fi

if [ -n "$requirements_file" ]; then
echo "Adding requirements..."
cp -L --preserve=timestamps "$requirements_file" "$tmpdir/requirements.yml"
cp -L -p "$requirements_file" "$tmpdir/requirements.yml"
l50 marked this conversation as resolved.
Show resolved Hide resolved
fi

if [ -n "$vars_file" ]; then
echo "Adding vars..."
cp -L --preserve=timestamps "$vars_file" "$tmpdir/vars.yml"
cp -L -p "$vars_file" "$tmpdir/vars.yml"
l50 marked this conversation as resolved.
Show resolved Hide resolved
fi

if [ ${#extra_deps[@]} -gt 0 ]; then
Expand All @@ -218,7 +218,7 @@ copy_playbook() {
fi

echo "Adding ansible config..."
cp -L --preserve=timestamps "$ETC_PATH/ansible.cfg" "$tmpdir"
cp -L -p "$ETC_PATH/ansible.cfg" "$tmpdir"
l50 marked this conversation as resolved.
Show resolved Hide resolved
}

add_python_requirements() {
Expand Down Expand Up @@ -290,20 +290,34 @@ install_roles() {

add_entrypoint() {
echo "Adding entrypoint..."
cp -L --preserve=timestamps "$LIB_PATH/run-playbook.sh" "$tmpdir"
chmod 0755 "$tmpdir/run-playbook.sh"
if [ -e "$LIB_PATH/run-playbook.sh" ]; then
Copy link
Owner

Choose a reason for hiding this comment

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

This check should not be here, instead we must evaluate that $LIB_PATH is set to the correct path when this runs - and this is already done at set_config_vars()

Copy link
Author

Choose a reason for hiding this comment

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

I'm afraid I don't understand your feedback here, but I'm happy to implement whatever proposed changes you have.

cp -L -p "$LIB_PATH/run-playbook.sh" "$tmpdir"
chmod 0755 "$tmpdir/run-playbook.sh"
else
echo "File not found: $LIB_PATH/run-playbook.sh"
fi
}

compress_output() {
local output_file=$1
local output_file=$1
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
local output_file=$1
local output_file=$1

Don't change the indentation. See .editorconfig for editor settings


echo "Packaging files to the output..."
echo "Packaging files to the output..."
l50 marked this conversation as resolved.
Show resolved Hide resolved

# First add the header to the final script
cat "$LIB_PATH/bin-header.sh" > "$output_file"
# First add the header to the final script
Copy link
Owner

Choose a reason for hiding this comment

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

This check should not be here, instead we must evaluate that $LIB_PATH is set to the correct path when this runs - and this is already done at set_config_vars()

Copy link
Author

Choose a reason for hiding this comment

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

I'm afraid I don't understand your feedback here, but I'm happy to implement whatever proposed changes you have.

if [ -e "$LIB_PATH/bin-header.sh" ]; then
cat "$LIB_PATH/bin-header.sh" > "$output_file"
else
echo "File not found: $LIB_PATH/bin-header.sh"
fi

SED_BINARY=''
# Do some build-time metadata insertion
sed -i'' \
if [[ "$(uname)" == 'Darwin' ]]; then
SED_BINARY=`which gsed`
Copy link
Owner

Choose a reason for hiding this comment

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

Here you're adding a hidden dependency on gnu-sed which implies the user will have it installed.

I know that the only thing that kept you from using sed here was the fact that -i flag is incompatible between the platforms. We can easily solve that by:

sed -i.bkp -e '...' "$output_file"; rm "${output_file}.bkp"

And we avoid the conditional and the hidden dependency.

Copy link
Author

@l50 l50 May 1, 2023

Choose a reason for hiding this comment

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

Added docs for installing gnu grep if on a Mac to the README. It saves on the temp file.

else
SED_BINARY=`which sed`
fi
$SED_BINARY -i'' \
-e "s/UNCOMPRESS_SKIP=.*/UNCOMPRESS_SKIP=$(( $(wc -l < "$output_file") + 1 ))/" \
-e "s/\$VERSION/$VERSION/g" \
"$output_file"
Expand Down
2 changes: 1 addition & 1 deletion app/lib/bin-header.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
#!/usr/bin/env bash
l50 marked this conversation as resolved.
Show resolved Hide resolved
#
# This is a packaged ansible playbook file using Ansible Bundler v$VERSION.
# You can run this with --debug to show more information about the process.
Expand Down
8 changes: 6 additions & 2 deletions app/lib/run-playbook.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
#!/usr/bin/env bash
l50 marked this conversation as resolved.
Show resolved Hide resolved
#
# This is the entry point for the playbook bundle. It has a hard dependency on Python. It will try
# to look for an already existing installation of Ansible, and will try to install it if not found.
Expand All @@ -11,7 +11,11 @@ main() {
export BASEDIR=${BASEDIR:-.}

# Ensure we have HOME defined, otherwise set it manually
test -z "$HOME" && export HOME; HOME="$(getent passwd "$(id -un)" | cut -d: -f6)"
if [[ "$(uname)" == 'Darwin' ]]; then
l50 marked this conversation as resolved.
Show resolved Hide resolved
test -z "$HOME" && export HOME; HOME="$(dscl . -read /Users/$(id -un) NFSHomeDirectory | awk -F': ' '{print $2}')"
l50 marked this conversation as resolved.
Show resolved Hide resolved
else
test -z "$HOME" && export HOME; HOME="$(getent passwd "$(id -un)" | cut -d: -f6)"
fi

export PIP_ROOT_PATH; PIP_ROOT_PATH="$(realpath "${BASEDIR}/python-deps")"
export PATH="${PIP_ROOT_PATH}/usr/bin:${PIP_ROOT_PATH}${HOME}/.local/bin:${PATH}"
Expand Down
12 changes: 12 additions & 0 deletions examples/basic.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---

- hosts: all

tasks:
- name: basic-example
debug:
msg: You are running a basic example

- name: Print extra variable 'example'
debug:
msg: "Example value is {{ example }}"
l50 marked this conversation as resolved.
Show resolved Hide resolved