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

Use public interface for git-repo-info #40

Closed kkumler closed 7 years ago

kkumler commented 7 years ago

What Changed & Why

The release of git-repo-info 1.3.0 changed the private function that was being used to obtain git information. The change broke deploys using git-commit, git-tag-commit, and version-commit.

Failure examples: https://travis-ci.org/kkumler/ember-cli-deploy-revision-data/jobs/168504161 TypeError: Arguments to path.join must be strings

Related issues

https://github.com/rwjblue/git-repo-info/pull/26#issuecomment-254361000

PR Checklist

Mention people who would be interested in the changeset (if any)

lukemelia commented 7 years ago

Looks good so far @kkumler!

kkumler commented 7 years ago

@lukemelia Great. What else -- if anything -- do you need from me for this?

lukemelia commented 7 years ago

@kkumler Would a regression test be possible?

ip2k commented 7 years ago

In case anyone needs a work-around for this right now, it's possible to pin git-repo-info to 1.2.0 in package.json:

"git-repo-info": "1.2.0",

OR switch to file-hash in config/deploy.js:

  ENV["revision-data"] = {
    //filePattern: '**/*.{html,js,css,png,gif,ico,jpg,map,xml,txt,svg,swf,eot,ttf,woff,woff2}',
    type: 'file-hash'
  };

Thanks for the prompt fix!

kkumler commented 7 years ago

@lukemelia I would imagine the existing tests are sufficient. If you run the tests on master (with git-repo-info 1.3.0), they will fail. cf. https://travis-ci.org/kkumler/ember-cli-deploy-revision-data/jobs/168504161

If there is a test that would add value, I'm more than happy to write it.

ip2k commented 7 years ago

Any chance on getting this merged soon? We're seeing issues again, now even with the workarounds we had been using previously:

+- prepare
|  |
|  +- revision-data
- creating revision data using `file-hash`
- TypeError: path must be a string or Buffer
|
+- didFail
TypeError: path must be a string or Buffer
TypeError: path must be a string or Buffer
  at TypeError (native)
  at Object.fs.statSync (fs.js:981:18)
  at FSMonitor._measure (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/heimdalljs-fs-monitor/index.js:66:21)
  at Object.statSync (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/heimdalljs-fs-monitor/index.js:82:30)
  at exists (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/simple-git/src/util/exists.js:7:21)
  at module.exports (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/simple-git/src/util/exists.js:28:11)
  at module.exports (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/simple-git/src/index.js:10:21)
  at /var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/ember-cli-deploy-revision-data/lib/scm-data-generators/git.js:15:7
  at initializePromise (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/rsvp/dist/rsvp.js:588:5)
  at PromiseExt.Promise (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/rsvp/dist/rsvp.js:1076:31)
  at new PromiseExt (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/ember-cli/lib/ext/promise.js:32:8)
  at CoreObject.module.exports.CoreObject.extend.generate (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/ember-cli-deploy-revision-data/lib/scm-data-generators/git.js:14:12)
  at Class.DeployPluginBase.extend._getScmData (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/ember-cli-deploy-revision-data/index.js:67:14)
  at Class.DeployPluginBase.extend.prepare (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/ember-cli-deploy-revision-data/index.js:37:23)
  at Object._pipeline.register.fn (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/ember-cli-deploy/lib/tasks/pipeline.js:159:21)
  at Pipeline._notifyPipelinePluginHookExecution (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/ember-cli-deploy/lib/models/pipeline.js:175:19)
  at tryCatch (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/rsvp/dist/rsvp.js:538:12)
  at invokeCallback (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/rsvp/dist/rsvp.js:553:13)
  at /var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/rsvp/dist/rsvp.js:628:16
  at flush (/var/lib/jenkins/jobs/Watson-canary-ui/workspace/node_modules/rsvp/dist/rsvp.js:2373:5)
  at _combinedTickCallback (internal/process/next_tick.js:67:7)
  at process._tickCallback (internal/process/next_tick.js:98:9)
|  |
|  +- slack
ericgoedtel commented 7 years ago

What workaround were you using? Pinning git-repo-info per @ip2k still seems to work.

ip2k commented 7 years ago

After many hours of fighting with this, we found that pinning simple-git to 1.58.0 fixes this for us. 1.59.0 breaks with the error I pasted above. With this new work-around, we no longer need to pin git-repo-info . I also opened https://github.com/steveukx/git-js/issues/132 .

ghedamat commented 7 years ago

@ip2k If I understand correctly #45 should fix this issue as well, that said I still think we're still interested in merging this one @kkumler

also @ericgoedtel in #44 we pinned git repo info already, are you using the latest version of ember-cli-deploy-revision-data?

thanks and sorry for the trouble!

kkumler commented 7 years ago

@ghedamat My comment above (https://github.com/ember-cli-deploy/ember-cli-deploy-revision-data/pull/40#issuecomment-255210730) still stands then. Running tests without this change, with git-repo-info 1.3.0, will fail.

ip2k commented 7 years ago

Update: When using "ember-cli-deploy-lightning-pack": "^0.6.7" , everything works, even without the "simple-git": "1.58.0" pin. https://github.com/ember-cli-deploy/ember-cli-deploy-lightning-pack/pull/29 was the fix, which updates "ember-cli-deploy-revision-data": "0.3.1" to "ember-cli-deploy-revision-data": "0.3.2" , which merged https://github.com/ember-cli-deploy/ember-cli-deploy-revision-data/pull/45 :) THANKS ALL!

ghedamat commented 7 years ago

seeems good @kkumler !

glad to hear it's ok for you @ip2k

now that this is merge the pinning will probably be dropped anyway