Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Node.js: handle multi-versions without repetition #197

Closed
wants to merge 1 commit into from

5 participants

Gilles Cornu Josh Kalderimis Nick Schonning Hiro Asari Henrik Hodne
Gilles Cornu
Collaborator

In continuation to #195, this change aims to simplify travis-boxes overrides for the Node.js
worker (corresponding PR is coming asap)

Gilles Cornu gildegoma Node.js: handle multi-versions without repetition
This change aims to simplify travis-boxes overrides for the nodejs
worker.
4494083
Josh Kalderimis
Owner

i'm not a big fan of [:others], can you show me what an example config might look like and what this transforms it to?

Gilles Cornu gildegoma referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Gilles Cornu gildegoma referenced this pull request in travis-ci/travis-boxes
Closed

Node.js: DRY with new cookbook defaults #17

Gilles Cornu
Collaborator

The DRY logic is well illustrated in travis-ci/travis-boxes#17. others could be renamed (e.g. other_versions, or whatever you prefer).

Another approach I considered, was to only rely on a single list in versions attribute, and by convention take the first element of this array as the default version. The drawback of this approach is that configurations in travis-boxes have to overwrite default cookbook attributes, and thus may differ and end up with workers provisioned with different default node version.

This change is not invasive in the cookbook recipe, but we also could consider update the recipe logic, but I don't think it is necessary...

Josh Kalderimis
Owner

Can we use :versions for a list, but also use :default_version or just :default ?

Nick Schonning
Collaborator

Wonder if you'd consider this travis-ci/travis-ci#1017 as part of this PR?

Gilles Cornu
Collaborator

@joshk: Sorry, I'm not sure to understand. Here, I only add some "in the middle" attribute logic to avoid value repetitions, with a "KISS" constraint to keep the recipe untouched.

I would like multi-node worker to be simply configured as following:

json:
  nodejs:
    others:
      # 0.10.x is default, inherited from cookbook default attributes.
      - 0.11.2
      - 0.9.12
      - 0.8.25
      - 0.6.21

Without others helper (aka additional_versions or whatever the name we could give it ;-) you also can configure as following:

json:
  nodejs:
    default: 0.10.12
    versions:
      - 0.11.2
      - 0.10.12
      - 0.9.12
      - 0.8.25
      - 0.6.21

It would result the same, but by overriding default, the Node.js VM is potentially provisioned with a different default version than other travis workers, which one of the DRY issue this PR aims to solve. Depends if you want to benefit of this DRY safety or not...

Note: I'm also thinking adding some safety check, to prevent version list to contain more than one minor version for the same major (e.g. 0.10.11 and 0.10.12). We could either fail with an exception or simply dropping the older releases. But I think it is overkill feature..., or it should be implemented as util toolkit (for any multi cookbook). As said... overkill!

@nschonni: Good you reactivate this point! I'd say first "no", according to travis-ci/travis-boxes#15 (comment), where a travis-build implementation was recommended. As mentioned there, node-head alias logic is different, since the target is the major alias, not a "true" release. Implementation at cookbook level is of course possible (and maybe accurate), but I would make it in a separate change/pull request. @joshk and @henrikhodne what do you think? It sounds not that bad idea to implement this "head" feature at cookbook level, since it is the place where we potentially update to a new head...

Josh Kalderimis
Owner

First off I wanted to say I like what you did with the alias code.

The thing I am not a fan of is the use of the name others in the config, which just doesn't sound right.

I can understand the default being set in the cookbook and merged into the versions config if it isn't present.

As for node-head, I don't think we should add this. node-head is just the latest odd numbered minor version (0.11.x right now) which we update when available. I know this is supposed to make upgrading node versions easier, but it also means you obscure what version you are really testing against. (since it is no longer obvious when looking at the build matrix)

Nick Schonning
Collaborator

Sorry, I don't want to thread-jack, but if the head/dev/other alias name is added, then the actual version number 0.9 and 0.11 release would be removed entirely. There is only a need for a single unstable release support at a time, because the unstable just graduates to stable.

Gilles Cornu
Collaborator

Sorry for misunderstanding your remarks and happy if we only have to solve naming cosmetics! As already said above, we'll name others as you want...

I actually agree that others is a "bad name". In fact, I was annoyed to select a "good name", because I wanted to keep original attributes unmodified (default and versions). At the moment versions attribute name is reserved, unless we further update recipes/multi.rb, which I initially wanted to avoid. Apart from that, versions would not be a correct name, since default version is not included in this "others" attribute.

Minimizing recipe code modifications, I'd propose to rename default as default_version and end up with something like:

nodejs:
  default_version: 0.10.12
  alternate_versions:
    - 0.11.2
    - 0.9.12
    ...

I proposed alternate_versions, but please give me your preferred name (why not additional_versions ?). Please @joshk make your choice... and I update accordingly.

Gilles Cornu gildegoma referenced this pull request in travis-ci/travis-boxes
Closed

Bump nodejs stable and unstable versions to latest #19

Henrik Hodne henrikhodne added the nodejs label
Hiro Asari
Owner

Is this still relevant, now that we are using nvm?

Gilles Cornu
Collaborator

So far, we have to be careful when updating travis-images:templates/worker.node-js.yml, to be coherent with travis-cookbooks default attributes, like illustrated in the following commits (set 0.10.29 as new default):

I think this little improvement is still relevant, but it has clearly "low priority". I'll close it for now, because it is outdated and might be revisited when we'll move to Chef roles (travis-ci/travis-images#8).

Note that the same principle should be applied to any other multi-version cookbooks that defines a default version (e.g. rvm/Ruby).

Gilles Cornu gildegoma closed this
Gilles Cornu gildegoma deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 21, 2013
  1. Gilles Cornu

    Node.js: handle multi-versions without repetition

    gildegoma authored
    This change aims to simplify travis-boxes overrides for the nodejs
    worker.
This page is out of date. Refresh to see the latest.
Showing with 13 additions and 3 deletions.
  1. +13 −3 ci_environment/nodejs/attributes/multi.rb
16 ci_environment/nodejs/attributes/multi.rb
View
@@ -1,3 +1,13 @@
-default[:nodejs][:default] = "0.10.12"
-default[:nodejs][:versions] = [ node[:nodejs][:default] ]
-default[:nodejs][:aliases] = { node[:nodejs][:default] => node[:nodejs][:default][/\d+\.\d+/] }
+default[:nodejs][:default] = '0.10.12'
+default[:nodejs][:others] = nil
+
+if node[:nodejs][:others].is_a?(Array)
+ default[:nodejs][:versions] = node[:nodejs][:others].insert(0, node[:nodejs][:default]).uniq
+else
+ default[:nodejs][:versions] = [ node[:nodejs][:default] ]
+end
+
+node[:nodejs][:versions].each do |nv|
+ default[:nodejs][:aliases][nv] = nv[/\d+\.\d+/]
+end
+
Something went wrong with that request. Please try again.