831771 - deploy node.js to Ubuntu

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task, P2)

Description

[Rail Aliiev [:rail]]

Needed for some SPDY tests.

Comment 1

[Rail Aliiev [:rail]]

Attachment: deploy nodejs

It turns out that the package is already in Ubuntu, so no need to package it.

It needs a symlink to satisfy http://hg.mozilla.org/build/buildbotcustom/file/ad9508ea1e6e/env.py#l80. The symlink can be removed once we get rid of Fedora 12 and update the corresponding env.

Comment 2

[Justin Wood (:Callek)]

Comment on attachment 706420 [details] [diff] [review]
deploy nodejs

>+++ b/modules/packages/manifests/nodejs.pp
>+class packages::httpd {

packages::nodejs

>diff --git a/modules/talos/manifests/init.pp b/modules/talos/manifests/init.pp
>+    case $::operatingsystem {
>+        Ubuntu: {
>+            include packages::nodejs
>+            file {

>+                "${users::builder::home}/bin/node.exe":
>+                    ensure => link,
>+                    target => "/usr/bin/node";

I  feel that the OS switch here should happen in packages::nodejs, we should include packages::nodejs always for talos slaves. And then symlink for it (as well as an include of dirs::builder_home::bin should happen there)

>+                "${users::builder::home}/bin":
>+                    ensure => directory,

Please factor out into dirs::builder_home::bin (so it can be used multiple places if need be - can even do a purge of non-tracked this way.)

Comment 3

[Rail Aliiev [:rail]]

Attachment: deploy nodejs

Let's go with a cleaner plan:

* No symlinks on Ubuntu
* symlink /home/cltbld/bin/node.exe to /usr/bin/node on Fedora (patch incoming)
* update env and point it to /usr/bin/node

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 706428 [details] [diff] [review]
deploy nodejs

Review of attachment 706428 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/packages/manifests/nodejs.pp
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +class packages::httpd {

still need to fix this packages::*  but r+ assuming you do fix it

Comment 5

[Rail Aliiev [:rail]]

Attachment: puppet-manifests

Deploy it to /usr/bin instead!

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 706428 [details] [diff] [review]
deploy nodejs

(In reply to Justin Wood (:Callek) from comment #4)
> still need to fix this packages::*  but r+ assuming you do fix it

Bah, fixed and pushed http://hg.mozilla.org/build/puppet/rev/f192ae381df5

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 706429 [details] [diff] [review]
puppet-manifests

even better than a symlink, however we *might* want to (after this lands and sticks) re-add that to ensure => absent to save the few bytes on the machines. Either way I'm ok if we don't.

Comment 8

[Rail Aliiev [:rail]]

Attachment: ENV

Depends on puppet-manifests patch deployment.

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 706429 [details] [diff] [review]
puppet-manifests

http://hg.mozilla.org/build/puppet-manifests/rev/810fff6c9606

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 706433 [details] [diff] [review]
ENV

We can't deploy this until we are sure the fedora manifests patch sticks, and is fully deployed. But I trust you to know when is the right time.

Comment 11

[Rail Aliiev [:rail]]

Comment on attachment 706433 [details] [diff] [review]
ENV

http://hg.mozilla.org/build/buildbotcustom/rev/d6a91d86930e

I checked fed/fed64 slaves, all of them except some preproduction, one waiting for reimaging and one waiting for reboot are done.

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Can you switch this around so that rather than
  include package::nodejs
not doing what it says on CentOS/Darwin, it just doesn't get invoked there?

That basically means adding a conditional in modules/talos, and removing the "N/A" stanza in pacakges::nodejs

Comment 13

[Justin Wood (:Callek)]

(In reply to Dustin J. Mitchell [:dustin] from comment #12)
> Can you switch this around so that rather than
>   include package::nodejs
> not doing what it says on CentOS/Darwin, it just doesn't get invoked there?
> 
> That basically means adding a conditional in modules/talos, and removing the
> "N/A" stanza in pacakges::nodejs

That was an ask specifically by me above. Can you provide the reason for this? (I told rail in IRC I could be convinced otherwise)

(If you have a reason other than personal aesthetics I'll say we should do it ahead of time, since I don't have a strong reason for keeping it this way)

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

The reason is that, as it stands, 'include packages::nodejs' lies on Darwin and CentOS -- it doesn't install the package.  This on the basis of unstated knowledge that you don't need nodejs on those systems.  Like functions, Puppet types should do what they say.

In this case, the decision as to which systems do and do not have node installed is a talos-related decision, so it makes sense to put that logic in the talos module.  A comment about it would be even better :)

I'm not sure what "ahead of time" means.

Comment 15

[Justin Wood (:Callek)]

(In reply to Dustin J. Mitchell [:dustin] from comment #14)
> The reason is that, ...

Ok lets do it.

> I'm not sure what "ahead of time" means.

I wrote that and even I don't know :(

Comment 16

[Rail Aliiev [:rail]]

Attachment: move it

Do you mean something like this?

Comment 17

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 706621 [details] [diff] [review]
move it

Yep, please also add a comment in this file and remove this stanza from nodejs.pp:
        CentOS, Darwin: {
            # N/A
        }

Comment 18

[Rail Aliiev [:rail]]

Comment on attachment 706621 [details] [diff] [review]
move it

http://hg.mozilla.org/build/puppet/rev/589f48a25a23

Comment 19

[Rail Aliiev [:rail]]

node.exe under ~/bin could be removed, but it's not a big deal since Fedora 12 platform will be dead soon.