-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: master
Are you sure you want to change the base?
Conversation
- 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
There was a problem hiding this 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.
etc_path=/etc/ansible-bundler | ||
bin_path=. | ||
lib_path=../lib/ansible-bundler | ||
etc_path=../../etc/ansible-bundler |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- Author wants people to use packages instead of building locally. By moving the installation instructions up higher in the README, new users to the project will have an easier time getting started.
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. |
@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. |
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:
It does generate output anyway. But afterwards, when trying to run:
|
This pull request contains the following:
Makefile
values to relative paths from the build directory