Return error if deploying Meteor runtime to node.js runtime #118

theworkflow · 2016-06-08T16:57:15Z

Return error if deploying to the wrong runtime
Return error if deploying an outdated version of Meteor to the Meteor runtime

Closes #117

franvarney · 2016-06-08T17:09:11Z

lib/controllers/project.js

+  var errMessage = 'You are deploying a %s project to a %s runtime. Please select the correct runtime.';
+  var minMeteorVersion = parseVersion(Project.supportedVersions.Meteor);
+
+  function parseVersion (str) {


This should be above where it's being called (line 221)

theworkflow · 2016-06-08T19:13:09Z

@franvarney updated

Return error if deploying an an outdated version of Meteor to the Meteor runtime

jackboberg · 2016-06-08T20:39:47Z

lib/controllers/project.js

+  */
+//-----------------------------------------------------------------------------
+Project.prototype.detectProjectType = function (path, callback) {
+  async.waterfall([


why is this a waterfall?

For when we check all other project types. I did this for future reference

deploying to correct runtime.

theworkflow · 2016-06-10T18:13:48Z

updated

theworkflow · 2016-06-10T18:17:55Z

I was thinking of moving all the code from line 236 to line 251 to a separate function that would check the version of whatever project type the user is deploying. What do you all think?

jackboberg · 2016-06-14T17:32:49Z

lib/controllers/project.js

+  * @param {Boolean} Returns boolean
+  */
+//-----------------------------------------------------------------------------
+Project.prototype.isSupported = function (srcVersion, runtime) {


Can you add some tests for this? I think I see logic that won't work, would like to confirm:

isSupported('2.0.0', 'Meteor') // return majorPass && minorPass && patchPass; return true && false && true; // false

maybe use a module like semver?

I can add some tests, but Meteor doesn't follow semver. This was discussed when testing the meteor version in the build image.

documenting comments IRL:

even if Meteor is not semver, this function is and should probably be replaced with semver.gte(srcVersion, minVersion)

Replaced custom compatibility check with semver

jackboberg · 2016-06-21T15:45:44Z

test/controllers/project.js

+      expect(supported).to.equal(true);
+      done();
+    });
+  });


add test for 'badly' formatted versions (eg: 1.3.2.4)

jackboberg · 2016-06-23T14:21:24Z

lib/controllers/project.js

@@ -761,8 +760,31 @@ Project.prototype.detectProjectType = function (path, callback) {
  */
 //-----------------------------------------------------------------------------
 Project.prototype.isSupported = function (srcVersion, runtime) {
+  var imageVersion, majorPass, minorPass, patchPass;


let's not rewrite semver, you can use that tested code by just handling the weird edge case for Meteor:

var input = 'METEOR@1.3.2.4' var srcVersion = input.replace('METEOR@', '') .split('.') .slice(0, 3) .join('.') // srcVersion === '1.3.2', ready for `semver.gte()`

This might be risky because of the way Meteor does versioning.. it's not semver. :/ I tried doing this in Demeteorizer for a while and had to make changes to the parsing logic to use semver every other Meteor release or so.

If this is risky, what's the best way to do this? Or is this goal not accomplished?

franvarney · 2016-06-27T13:54:58Z

lib/controllers/project.js

+    function (files, next) {
+      if (files.indexOf('.meteor') !== -1) {
+        fs.readdir(path.join(dir, '.meteor'), next);
+      } else next('ok', false);


Why are you passing 'ok' then at line 794 setting err = null?

short circuit the waterfall without passing an actual error. i.e.: stop running but don't fail

franvarney · 2016-06-27T14:13:24Z

lib/controllers/project.js

+    }
+  ], function (err, result) {
+    if (err === 'ok') err = null;
+    callback(err, result);


Couldn't you just pass null and ignore err?

passing null means the waterfall calls the next function, this is an escape hatch

franvarney · 2016-06-27T14:28:40Z

👍

musgravejw · 2016-07-05T15:44:06Z

PR looks ok to me. Maybe @jackboberg could confirm that issues have been addressed.

jackboberg · 2016-07-05T19:47:08Z

👍 all my local tests performed as expected

dsjoerg · 2016-08-12T10:52:36Z

I have a meteor 1.2 project that was happily running on modulus as a node.js runtime. Now when I go to deploy it, it's yelling at me about "You are deploying a Meteor project to a Node.js runtime". Is there a way around this?

theworkflow · 2016-08-12T13:15:34Z

@dsjoerg Unfortunately you will have to downgrade your CLI. If you have a Meteor runtime option available, we recommend creating a new project under the Meteor runtime and deploying your source. If you prefer to not go this route, you will need to downgrade your CLI so the demeteorizer process happens locally.

dsjoerg · 2016-08-12T14:05:33Z

Thanks @HarlanJ that worked (after I found out how to downgrade CLI, Googling for downgrade meteor CLI gets you nowhere :\ )

theworkflow added the enhancement label Jun 8, 2016

theworkflow assigned jackboberg, musgravejw, theworkflow, tzmanics and franvarney Jun 8, 2016

tzmanics added the in progress label Jun 8, 2016

franvarney reviewed Jun 8, 2016
View reviewed changes

theworkflow force-pushed the enhancement/check-runtime branch from 9090298 to bfb7ad7 Compare June 8, 2016 19:12

theworkflow force-pushed the enhancement/check-runtime branch from bfb7ad7 to 2fd8d28 Compare June 8, 2016 19:49

Return error if deploying to the wrong runtime.

2fd8d28

Return error if deploying an an outdated version of Meteor to the Meteor runtime

jackboberg reviewed Jun 8, 2016
View reviewed changes

Abstract functionality to check Meteor version and ensure user is

f99f498

deploying to correct runtime.

jackboberg reviewed Jun 14, 2016
View reviewed changes

Use semver to test srcVersion compatibility and add tests

8bc831a

theworkflow force-pushed the enhancement/check-runtime branch from 8955fe4 to 8bc831a Compare June 20, 2016 19:54

jackboberg reviewed Jun 21, 2016
View reviewed changes

Add more testing

949cb39

jackboberg reviewed Jun 23, 2016
View reviewed changes

theworkflow force-pushed the enhancement/check-runtime branch from c8c1f38 to f1b94d0 Compare June 24, 2016 14:05

update code to pass tests

6d070f2

theworkflow force-pushed the enhancement/check-runtime branch from f1b94d0 to 6d070f2 Compare June 24, 2016 14:05

franvarney reviewed Jun 27, 2016
View reviewed changes

theworkflow changed the title ~~Return error if deploying to the wrong runtime.~~ Return error if deploying Meteor runtime to node.js runtime Jul 5, 2016

theworkflow merged commit 4d3e76c into master Jul 5, 2016

theworkflow deleted the enhancement/check-runtime branch July 5, 2016 20:25

tzmanics removed the in progress label Jul 5, 2016

jackboberg unassigned musgravejw, tzmanics and franvarney Dec 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return error if deploying Meteor runtime to node.js runtime #118

Return error if deploying Meteor runtime to node.js runtime #118

theworkflow commented Jun 8, 2016

franvarney Jun 8, 2016

theworkflow commented Jun 8, 2016

jackboberg Jun 8, 2016

theworkflow Jun 8, 2016

theworkflow commented Jun 10, 2016

theworkflow commented Jun 10, 2016

jackboberg Jun 14, 2016 •

edited

jackboberg Jun 14, 2016 •

edited

theworkflow Jun 15, 2016

jackboberg Jun 15, 2016 •

edited

theworkflow Jun 20, 2016

jackboberg Jun 21, 2016

jackboberg Jun 23, 2016

fiveisprime Jun 23, 2016

theworkflow Jun 23, 2016

franvarney Jun 27, 2016

jackboberg Jun 27, 2016

franvarney Jun 27, 2016

jackboberg Jun 27, 2016 •

edited

franvarney commented Jun 27, 2016

musgravejw commented Jul 5, 2016

jackboberg commented Jul 5, 2016

dsjoerg commented Aug 12, 2016

theworkflow commented Aug 12, 2016

dsjoerg commented Aug 12, 2016

Return error if deploying Meteor runtime to node.js runtime #118

Return error if deploying Meteor runtime to node.js runtime #118

Conversation

theworkflow commented Jun 8, 2016

Choose a reason for hiding this comment

theworkflow commented Jun 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theworkflow commented Jun 10, 2016

theworkflow commented Jun 10, 2016

jackboberg Jun 14, 2016 • edited

Choose a reason for hiding this comment

jackboberg Jun 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackboberg Jun 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackboberg Jun 27, 2016 • edited

Choose a reason for hiding this comment

franvarney commented Jun 27, 2016

musgravejw commented Jul 5, 2016

jackboberg commented Jul 5, 2016

dsjoerg commented Aug 12, 2016

theworkflow commented Aug 12, 2016

dsjoerg commented Aug 12, 2016

jackboberg Jun 14, 2016 •

edited

jackboberg Jun 14, 2016 •

edited

jackboberg Jun 15, 2016 •

edited

jackboberg Jun 27, 2016 •

edited