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

Initial macOS ansible scripts #971

Closed
wants to merge 1 commit into from
Closed

Initial macOS ansible scripts #971

wants to merge 1 commit into from

Conversation

gdams
Copy link
Member

@gdams gdams commented Nov 3, 2017

Okay so here are my initial ansible scripts for macOS. Obviously the inventory isn't complete yet but I will work with @mhdawson to get that side ready. I have confirmed that this works with a blank machine from macstadium (also ran a test build to confirm all tools were in place).

Things to note:

this requires NOPASSWD to be added to the sudoers file to enable elevation

sudo visudo
%admin          ALL = (ALL) NOPASSWD:ALL

Possible things to add?

do we want to disable the firewall on the test machines to prevent tonnes of popups?

mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

gibfahn

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

@joaocgreis
Copy link
Member

I don't think start.sh should be in all machines. There should be some kind of mechanism everywhere to restart Jenkins when it crashes, which is (or was) a relatively frequent issue. So, if Jenkins does not start because of the restart mechanism, we should debug the restart mechanism. If Jenkins starts and we want it stopped for some reason, we must always disable the restart mechanism anyway. Having start.sh everywhere will be very misleading and may not be kept in sync with the other way Jenkins is launched.

start.sh was traditionally used with monit. Here, I see it used with launchctl and a KeepAlive=true config entry. If that does not work, then we should debug it, not start Jenkins manually and risk having two running.

About this PR, shouldn't the two files under ansible/partials/ be in ansible/roles/package-upgrade/files/ instead, since they're only required for package-upgrade?

This shouldn't land with the current inventory. A partial one is ok, or even no inventory entries. Also, Gibson raised a valid point about the duplicated sudo entry. After these are addressed LGTM. Thanks for doing this!

@gibfahn
Copy link
Member

gibfahn commented Nov 8, 2017

I don't think start.sh should be in all machines. There should be some kind of mechanism everywhere to restart Jenkins when it crashes, which is (or was) a relatively frequent issue. So, if Jenkins does not start because of the restart mechanism, we should debug the restart mechanism. If Jenkins starts and we want it stopped for some reason, we must always disable the restart mechanism anyway. Having start.sh everywhere will be very misleading and may not be kept in sync with the other way Jenkins is launched.

I was thinking that maybe ~iojs/start.sh could just call the relevant service restart command (to avoid people having to remember what it is for each platform). Might be confusing to call it start.sh in that case though.

@joaocgreis
Copy link
Member

I was thinking that maybe ~iojs/start.sh could just call the relevant service restart command

That would be quite nice! The jenkins-worker role should probably be separated by the boot system into partials, perhaps with a common section. It would be nice to include a start.sh (or restart?) script that does the right thing there, different for each start system.

@mhdawson
Copy link
Member

Agreed it would be nice if there was a script that could be used to start/stop which calls the version for the specific platform. Maybe "agent-control" with start and stop options ?

mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

refack

This comment was marked as off-topic.

@refack
Copy link
Contributor

refack commented Dec 1, 2017

Can we land this, and iterate later? On just land the inventory?

@refack
Copy link
Contributor

refack commented Dec 1, 2017

BTW the libuv jobs need GYP to be in ~/gyp so meanwhile I added a curl call in the job:
https://ci.nodejs.org/job/libuv-test-commit-osx/jobConfigHistory/showDiffFiles?timestamp1=2017-11-22_22-16-55&timestamp2=2017-12-01_15-30-59

refack

This comment was marked as off-topic.

@mhdawson
Copy link
Member

mhdawson commented Dec 1, 2017

I'm all for landing minus the inventory and then updating once we have additional changes (like adding in support for ccache)

@gibfahn
Copy link
Member

gibfahn commented Dec 1, 2017

I'm all for landing minus the inventory and then updating once we have additional changes (like adding in support for ccache)

SGTM, but it'd be good to add some TODOs to the source code (like for adding ccache) so we've noted the gaps.

joaocgreis

This comment was marked as off-topic.

@mhdawson
Copy link
Member

It was just waiting on a fix up for cache. But I'm +1 for landing and then fixing that

@gdams
Copy link
Member Author

gdams commented Feb 20, 2018

So we are able to easily install ccache. My only remaining issue is adding ccache to the path of the jenkins node. I added the path to ccache in etc/paths and this works locally but I am unable to make this extra path appear in jenkins. (I have even tried restarting the machine)

@gibfahn
Copy link
Member

gibfahn commented Feb 21, 2018

So we are able to easily install ccache. My only remaining issue is adding ccache to the path of the jenkins node. I added the path to ccache in etc/paths and this works locally but I am unable to make this extra path appear in jenkins. (I have even tried restarting the machine)

@gdams is /etc/paths definitely added even when things are launched directly (not through a terminal)?

@gdams
Copy link
Member Author

gdams commented Feb 21, 2018

is /etc/paths definitely added even when things are launched directly (not through a terminal)?

@gibfahn I'm not 100% sure. I'll do some investigation.

@rvagg
Copy link
Member

rvagg commented Apr 20, 2018

I needed the macstadium hosts in my ssh config and @Trott couldn't figure it out to go fix some broken hosts so I've pulled out the inventory.yml and plugin fixes here and pushed them to master as 0495bc2. It'd be good if we could get this merged soon, it can be iterated on once merged if it's not fully working yet.

@mhdawson
Copy link
Member

I'm +1 on merging and then updating if we need to. @gdams can you rebase and then we'll do that?

@gibfahn
Copy link
Member

gibfahn commented Apr 22, 2018

It'd be good if we could get this merged soon, it can be iterated on once merged if it's not fully working yet.

Yep, we an just leave TODOs where things aren't working yet (or need manual setup).

@gdams
Copy link
Member Author

gdams commented Apr 23, 2018

updated PTAL

mhdawson

This comment was marked as off-topic.

@gdams
Copy link
Member Author

gdams commented Apr 24, 2018

@mhdawson I think those merge issues are now cleared up

mhdawson

This comment was marked as off-topic.

@mhdawson
Copy link
Member

Ok, will try to land by the end of week unless there are objections, comments from other @nodejs/build team members.

@mhdawson
Copy link
Member

@gdams can you squash your commits. git am is failing with conflicts even with -3. I suspect that if you squash that will get rid of the issue.

@gdams gdams force-pushed the gdams branch 2 times, most recently from b8e5858 to dbbea51 Compare May 1, 2018 21:10
@gdams
Copy link
Member Author

gdams commented May 1, 2018

@mhdawson rebased and squashed

mhdawson pushed a commit that referenced this pull request May 1, 2018
PR-URL: #971
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@mhdawson
Copy link
Member

mhdawson commented May 1, 2018

Landed as eb68f59

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.

7 participants