-
Notifications
You must be signed in to change notification settings - Fork 29
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; Add Windows support; And more... #33
base: master
Are you sure you want to change the base?
Conversation
@gildegoma please review. Pull request #32 is necessary for this PR |
recipes/unix.rb
Outdated
prefix_root node['android-sdk']['setup_root'] | ||
prefix_home node['android-sdk']['setup_root'] | ||
owner node['android-sdk']['owner'] | ||
group node['android-sdk']['group'] unless mac_os_x? |
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.
Don't think this should be forced to be unless mac_os_x
. Takes control away from downstream cookbooks.
recipes/unix.rb
Outdated
environment 'ANDROID_HOME' => android_home | ||
path [File.join(android_home, 'tools')] | ||
user node['android-sdk']['owner'] | ||
group node['android-sdk']['group'] unless mac_os_x? |
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.
I suspect this is strictly because chef-client
might be run as root with wheel
group, which is the tmp script owner?
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.
as previous PR i will remove limitation for mac_os_x
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.
Ah, k, wasn't sure if I had messed up. If I even set user/group to anything other than root
/ wheel
I always get error:
STDERR: couldn't read file "/var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/chef-script20170125-55984-udq962": permission denied
Still tracking down why that might be :(
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.
use http://www.rubydoc.info/github/opscode/chef/Chef/Mixin/HomebrewUser#find_homebrew_uid-instance_method :) this should solves your problem with user
.
@rjaros87 @Brantone Sorry guys for being so unresponsive... This PR contains a lot of different things, making the review more difficult and time consuming (and I lack of large bunch of spare time). I'll do my best to reserve some time for starting soon though. Do you have by the way some good Packer templates to build local VMs for testing on Windows and Mac (Virtualbox, Parallels or VMware welcome, or recommendation for a good Cloud provider, ideally well supported by Vagrant plugin ecosystem)? ❤️ ❤️ ❤️ |
recipes/unix.rb
Outdated
only_if { node['android-sdk']['set_environment_variables'] } | ||
end | ||
elsif mac_os_x? | ||
bash_profile 'profile.android_sdk' do |
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.
Getting mixed results with ~/.bash_profile
when sudo su
to android-sdk user.
Not sure if Sierra is hitting the order of sourcing bash files differently.
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.
Just test it :) using Vagrant & Virtualbox & .kitchen
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.
Yup, that's what I was doing. In simple kitchen test it's fine, it's when it's integrated with an environment cookbook. More specifically, when logging in as one user, then sudoing to another. I actually think the export PATH
online 116 could probably go in /etc/paths.d/android-sdk
then it's system wide, not just to specific user.
@gildegoma I use Vagrant + VirtualBox (with extension pack) and Kitchen :) as you know everything is in .kitchen.yml For testing Windows you need also Vagrant plugin for WinRM |
@rjaros87 Sorry, I missed the fact that your last commit (04dd45b) added two new kitchen boxes ( |
@gildegoma |
Remove macOS group constraint and bump some dep versions
@gildegoma ready to review. Please close other pull request and make review this one. |
Thank you @rjaros87 for all these propositions. As mentionned, having all these different things packed together is not the ideal way to get things merged quickly. I really value your efforts and work, and will do my best to do this review "asap", and hopefully take in most (if not all) your changes. Here is a short and recent article about the "hard work" to maintain stable projects: https://www.jeffgeerling.com/blog/2016/why-i-close-prs-oss-project-maintainer-notes But as already said above, I'm glad you made all this work and I am looking forward to integrate it here ❤️ ❤️ ❤️ |
@gildegoma did you disable travis tests? |
Please review my last changes. |
8703ef2
to
c602f96
Compare
Whats the status on this? Would be nice to use it from the supermarket. |
Adding my voice (again) to the "status?" pile. |
@gildegoma Been a long while since this has been open ... what can be done to move it along? |
Anyone? |
Only @gildegoma could merge this, no one have a permissions. |
Major changes:
Minor changes: