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

Conversation

l50
Copy link

@l50 l50 commented Apr 26, 2023

This pull request contains the following:

  • The program now works on macOS
  • Included brew formulae and instructions to install
    • You will want to change the values in the rb file to match your release, sha, etc.
  • There is input validation in place to stop the program from running when something isn't working
  • Added missing input validation to ansible example
  • Changed the Makefile values to relative paths from the build directory
  • Created an example and provided documentation in the README

@l50 l50 changed the title Add macOS support Add macOS support; input validation Apr 26, 2023
- The program now works on macOS
- There is input validation in place to stop the program from running when something isn't working
- Changed the Makefile values to relative paths from the build directory
@l50 l50 changed the title Add macOS support; input validation Add macOS support; input validation; an example Apr 26, 2023
Copy link
Owner

@kriansa kriansa 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 the contribution! It's really lovely to see that it's usable for macOS as well!

I understand that some of the changes you've made here are also to make it possible to run this project from the root path, without needing to install it.

While running it directly from your clone is acceptable for development reasons (i.e. you are developing ansible-bundler), it is a poor choice when it comes to using the software (i.e. developing your playbook), ideally, we should use a packaged version.

For a newly supported OS, I think the best is actually to ship it in a format usable by the OS, and for this specific case, I'm thinking of a homebrew formula. While this is out of the scope, it is the way forward for shipping it for macOS.

README.md Outdated Show resolved Hide resolved
app/bin/bundle-playbook Outdated Show resolved Hide resolved
app/bin/bundle-playbook Outdated Show resolved Hide resolved
app/bin/bundle-playbook Outdated Show resolved Hide resolved
app/bin/bundle-playbook Outdated Show resolved Hide resolved
app/lib/run-playbook.sh Show resolved Hide resolved
app/lib/run-playbook.sh Show resolved Hide resolved
app/lib/run-playbook.sh Outdated Show resolved Hide resolved
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.

Makefile Show resolved Hide resolved
@l50 l50 requested a review from kriansa May 1, 2023 03:56
@l50
Copy link
Author

l50 commented May 3, 2023

Following up to see what else is required to land the PR.

I'm adding some functionality to this project for my own use and figured I'd ship you the stuff that may be helpful.

@kriansa
Copy link
Owner

kriansa commented May 4, 2023

@l50 Hi Jayson,

I apologize for the lack of response, I saw your commits but I didn't have the time to review it yet. I'll do it this weekend.

@mattvonrocketstein
Copy link

I've been testing this also, both for OSX compatibility and because having a known-working example and usage without system installation would be nice (I can't install the packages on ubuntu, see #15 ).

For OSX during build I see an issue with non-gnu touch:

./bundle-playbook -f ../../../../examples/basic.yaml
Ansible Bundler - v1.10.2

Adding playbook...
Adding ansible config...
Adding entrypoint...
Packaging files to the output...
touch: illegal option -- d
usage:
touch [-A [-][[hh]mm]SS] [-acfhm] [-r file] [-t [[CC]YY]MMDDhhmm[.SS]] file ...
Bundle successfully packaged to ../../../../examples/basic.run
Done.

It does generate output anyway. But afterwards, when trying to run:

../../../../examples/basic.run
/tmp/ansible-bundle.5Nicp/run-playbook.sh: line 21: realpath: command not found
Installing playbook Python dependencies...
mkdir: .: No such file or directory
Collecting ansible (from -r /tmp/ansible-bundle.5Nicp/requirements.txt (line 1))
  Downloading https://files.pythonhosted.org/packages/fd/f8/071905c6a67592d0852a9f340f6ab9226861eeeb97fdf4068642b22edcf3/ansible-4.10.0.tar.gz (36.8MB)
    100% |████████████████████████████████| 36.8MB 6.5MB/s
[snip]
Successfully installed MarkupSafe-2.1.3 PyYAML-6.0 ansible ansible-core cffi-1.15.1 cryptography-41.0.1 jinja2-3.1.2 packaging-23.1 pycparser-2.21 resolvelib-0.5.5
You are using pip version 19.0.3, however version 23.1.2 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
find: : No such file or directory
/tmp/ansible-bundle.5Nicp/run-playbook.sh: line 114: ansible-playbook: command not found

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