Skip to content

Conversation

@cball
Copy link
Contributor

@cball cball commented Apr 23, 2016

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

@cball cball force-pushed the handle-empty-git-info branch 2 times, most recently from b911e70 to 677fba5 Compare April 23, 2016 21:48
@lukemelia
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

cball commented Apr 27, 2016

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

@seawatts
Copy link

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

@lukemelia
Copy link
Contributor

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

@cball cball force-pushed the handle-empty-git-info branch from 677fba5 to 60076b1 Compare April 28, 2016 14:39
@cball
Copy link
Contributor Author

cball commented Apr 28, 2016

@ghedamat added the requested changes.

if (sha) {
versionString = versionString + '+' + sha;
} else {
log('Missing git commit sha, using package version as revisionKey');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cball could you add { color: 'yellow', verbose: true } here?

example https://github.com/ember-cli-deploy/ember-cli-deploy-plugin/blob/master/index.js#L33

@lukemelia this also reminds me that we can set log color for info and danger but not warning levels (it's actually not really a level yet though, so yellow should do for now)

@ghedamat
Copy link
Contributor

@cball added another tiny request, after that will merge

Thanks again!

On some host/git combinations, gitRepoInfo returns an object like the
following:

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

When using the version-commit option, ember-cli-deploy would fail if the sha
is empty.
@cball cball force-pushed the handle-empty-git-info branch from 60076b1 to 8c43f50 Compare April 28, 2016 14:52
@cball
Copy link
Contributor Author

cball commented Apr 28, 2016

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

@ghedamat ghedamat merged commit 51910bc into ember-cli-deploy:master Apr 28, 2016
@ghedamat
Copy link
Contributor

perfect! thanks again!

@cball cball deleted the handle-empty-git-info branch April 28, 2016 15:03
@ghedamat
Copy link
Contributor

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

@seawatts
Copy link

@seawatts
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor Author

cball commented Apr 28, 2016

@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
Copy link
Contributor

lukemelia commented Apr 28, 2016

@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
Copy link
Contributor

same here :/ sry @cball

@cball
Copy link
Contributor Author

cball commented Apr 28, 2016

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants