ember-cli-deploy / ember-cli-deploy-revision-data

An ember-cli-plugin to create a unique revision key for the build
MIT License
21 stars 58 forks source link

Gracefully handle partial git info. #33

Closed cball closed 8 years ago

cball commented 8 years ago

On some host/git combinations (in my case CircleCI), gitRepoInfo seems to return an object like the following:

{ sha: null, abbreviatedSha: null, branch: 'develop', tag: null }

When using the version-commit option, ember-cli-deploy fails if the sha is empty. It's not clear to me what the proper behavior should be in this case.

A few options: 1) add just the package.json version and only add the sha if it can be read (this PR) 2) make a new data-generator that is package version only

lukemelia commented 8 years ago

Thanks @cball. We've had a few reports of people running into similar issues. @ghedamat can you review and share your opinion on Chris' question in the PR?

ghedamat commented 8 years ago

@cball I'm ok with this change as we still produce a valid version even without the git data and it's probably less confusing for the user instead of getting a weird crash.

Maybe we can add a note to the README to stress that some build env might not provide git data?

Might be also good to add a warning log message when those properties are null so that if someone uses --verbose they'll get some output.

cball commented 8 years ago

@ghedamat 👍 on the note to the README and warning log message, I'll check that out.

seawatts commented 8 years ago

Do we have an estimate on when this will get in and published? I'm also getting this error

lukemelia commented 8 years ago

@seawatts can you confirm that this PR fixes your problem? That would help give it momentum.

cball commented 8 years ago

@ghedamat added the requested changes.

ghedamat commented 8 years ago

@cball added another tiny request, after that will merge

Thanks again!

cball commented 8 years ago

@ghedamat sure thing! Totally missed that log took additional arguments... should be all set now.

ghedamat commented 8 years ago

perfect! thanks again!

ghedamat commented 8 years ago

@cball @seawatts , @lukemelia just released 0.2.2 :)

seawatts commented 8 years ago

@ghedamat thanks so much! Check this out! https://github.com/simplymeasured/ember-cli-deploy-sm-pack/pull/6

https://greenkeeper.io/ for the win!

seawatts commented 8 years ago

Hmm, getting this error now in circle.ci

TypeError: Cannot read property 'actualOutputStream' of undefined
TypeError: Cannot read property 'actualOutputStream' of undefined
TypeError: Cannot read property 'actualOutputStream' of undefined
    at CoreObject.extend.log (/home/ubuntu/xxx/node_modules/ember-cli-deploy-plugin/index.js:63:26)
    at /home/ubuntu/xxx/node_modules/ember-cli-deploy-revision-data/lib/data-generators/version-commit.js:39:11
    at lib$rsvp$$internal$$tryCatch (/home/ubuntu/xxx/node_modules/rsvp/dist/rsvp.js:1036:16)
    at lib$rsvp$$internal$$invokeCallback (/home/ubuntu/xxx/node_modules/rsvp/dist/rsvp.js:1048:17)
    at lib$rsvp$$internal$$publish (/home/ubuntu/xxx/node_modules/rsvp/dist/rsvp.js:1019:11)
    at lib$rsvp$asap$$flush (/home/ubuntu/xxx/node_modules/rsvp/dist/rsvp.js:1198:9)
    at nextTickCallbackWith0Args (node.js:453:9)
    at process._tickDomainCallback (node.js:423:13)Pipeline aborted
ghedamat commented 8 years ago

@seawatts :/ If I read that correctly it means that ui is undefined here https://github.com/ember-cli-deploy/ember-cli-deploy-plugin/blob/master/index.js#L63

does it work locally for you? we might have just introduced a bug that didn't get caught by the tests..

cball commented 8 years ago

@seawatts @ghedamat this is pretty strange, but I've verified the following change in an ssh build works:

+++ b/lib/data-generators/version-commit.js
@@ -22,7 +22,7 @@ module.exports = CoreObject.extend({

     var info = gitRepoInfo(path);
     var sha = (info.sha || '').slice(0, 8);
-    var log = this._plugin.log;
+    var plugin = this._plugin;

     return readFile(versionFile)
       .then(function(contents) {
@@ -36,7 +36,7 @@ module.exports = CoreObject.extend({
         if (sha) {
           versionString = versionString + '+' + sha;
         } else {
-          log('Missing git commit sha, using package version as revisionKey', { color: 'yellow', verbose: true });
+          plugin.log('Missing git commit sha, using package version as revisionKey', { color: 'yellow', verbose: true });
         }

         return {
lukemelia commented 8 years ago

@cball, this is definitely a bug. log will be invoked without the plugin as its this. Do you have time to put up a quick PR? I can follow with a quick release, since we've just broken lots of peoples deploys. Sorry I missed this in the review.

ghedamat commented 8 years ago

same here :/ sry @cball

cball commented 8 years ago

@lukemelia @ghedamat apologies I missed this the first time around. I just opened a PR with the fix mentioned above. On the (slight) plus side I think this would only have broken deploys that would have failed previously for not having an sha present.