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; Add Windows support; And more... #33

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

Conversation

rjaros87
Copy link
Contributor

Major changes:

Minor changes:

  • Unsupport Centos OS < 7 due to Android SDK use GNU C Library (glibc) 2.14 or later
  • Cleanup default['android-sdk']['components']

@rjaros87
Copy link
Contributor Author

@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?

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?

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?

Copy link
Contributor Author

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

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 :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gildegoma
Copy link
Owner

gildegoma commented Jan 25, 2017

@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

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.

Copy link
Contributor Author

@rjaros87 rjaros87 Jan 25, 2017

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

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.

@rjaros87
Copy link
Contributor Author

@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 vagrant plugin install vagrant-winrm

@gildegoma
Copy link
Owner

@rjaros87 Sorry, I missed the fact that your last commit (04dd45b) added two new kitchen boxes (mac_el_capitan and windows-2012R2). I'll test all this together, but as already said, I appreciate if in the future you can please create PR focused on a single specific addition.
Looking forward though, thanks again for all the work and your patience 💟

@rjaros87
Copy link
Contributor Author

rjaros87 commented Jan 25, 2017

@gildegoma
#29 - 7 file to review
#32 15 files - 7 files = 8 files to review
this #33 20 files - 15files = 5 files to review

@rjaros87
Copy link
Contributor Author

rjaros87 commented Feb 2, 2017

@gildegoma ready to review. Please close other pull request and make review this one.

@gildegoma
Copy link
Owner

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 gildegoma changed the title Add Windows support Add macOS support; Add Windows support; And more... Feb 2, 2017
@rjaros87
Copy link
Contributor Author

rjaros87 commented Mar 4, 2017

@gildegoma did you disable travis tests?
After merge with above branch, nothing triggered.

@gildegoma gildegoma added this to the 0.3.0 milestone Mar 4, 2017
@gildegoma
Copy link
Owner

@rjaros87 Yup, I temporarily disabled Travis CI (see #37) to avoid generating broken builds while preparing 0.2.1 release. This is active again. Please rebase onto an updated master and repush. The build should start again...

@rjaros87
Copy link
Contributor Author

rjaros87 commented Mar 5, 2017

Please review my last changes.
@gildegoma maybe you should create a 0.2 branch from master and merge this PR. What do you think?
Because it's hard to maintain and resolve conflicts. In some commits you done the same as I couple months ago.

@rjaros87 rjaros87 force-pushed the windows branch 2 times, most recently from 8703ef2 to c602f96 Compare May 29, 2017 08:34
@joshrlesch
Copy link

joshrlesch commented Aug 15, 2017

Whats the status on this? Would be nice to use it from the supermarket.

@Brantone
Copy link

Adding my voice (again) to the "status?" pile.

@Brantone
Copy link

@gildegoma Been a long while since this has been open ... what can be done to move it along?

@Brantone
Copy link

Anyone?

@rjaros87
Copy link
Contributor Author

Only @gildegoma could merge this, no one have a permissions.
Let say, I'm no more involved with Chef, so this is my last commit ;)

@Brantone
Copy link

@gildegoma ?

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

Successfully merging this pull request may close these issues.

4 participants