micha149 / gulp-maven-deploy

Gulp wrapper for the maven-deploy plugin
MIT License
12 stars 11 forks source link

`install` should also use the provided file #14

Closed ankon closed 8 years ago

ankon commented 8 years ago

8 was implemented (yeah!), but unfortunately only for the deploy plugin. The same change would also be reasonable for the install plugin, with pretty much the same arguments as in #8 for deploy, and now even more so to avoid confusion when working with both install and deploy.

micha149 commented 8 years ago

Sorry.I had it on my list, but not finished yet. Thanks for your help… so far. I'll look into your PR later and will merge this if everything is fine…

micha149 commented 8 years ago

Hey @ankon, yesterday I released version 1.0.0-beta.5 wich brings the announced better gulp behavior. Last week I finished my work on #5 which also affects the install() handling. Would you like to check if all works fine for you? If it does, I would like to release version 1.0.0 in a week, if no bugs occur.

ankon commented 8 years ago

@micha149: The good news first, for this ticket I think everything works, and the provided artifact is used.

The bad news: something seems off with the artifactId. Running this task (with _artifactName being a function to return the prepackaged artifact):

gulp.task('maven:install', [ 'maven:assemble', 'maven:setup' ], function () {
  console.log(this.project.groupId);
  console.log(this.project.artifactId);
  console.log(this.project.version);
  return gulp.src(_artifactName(this))
    .pipe($.mavenDeploy.install({
      groupId: this.project.groupId,
      artifactId: this.project.artifactId,
      version: this.project.version,
      type: this.project.packaging
    }));
}.bind(this));

Output is this, note that the artifactId parameter to maven is wrong and includes the version:

com.collaborne.app
collaborne-mobile-app
1.1.0-SNAPSHOT
Executing command: mvn -B install:install-file -Dpackaging=war -Dfile=/tmp/s-11642-7583-icnmqs -DgroupId=com.collaborne.app -DartifactId=collaborne-mobile-app-1.1.0-SNAPSHOT -Dversion=1.1.0-SNAPSHOT -DgeneratePom=true

I'm also pondering about generatePom there, but that's a different story I'm still looking into. :)

ankon commented 8 years ago

Ok: the reason for that is that the file we're creating is actually called collaborne-mobile-app-1.1.0-SNAPSHOT.war, and src/util/buildFileOptions.js uses the file.stem as artifact id -- ignoring the configuration.

https://github.com/micha149/gulp-maven-deploy/pull/16 fixes that. I think it would be good to spell out that default somewhere in the documentation though :)

micha149 commented 8 years ago

It has been documented in the README in the migration guide:

Remove artifactId and type from config. They are now extracted from the file name. To influence them, rename the file in the gulp stream before piping it to gulp-maven-deploy

It was my intention to overwrite artifactId and type from the config, because it should change for each file. If you pass multiple files to maven-deploy, you don't want to publish multiple artifacts with same ID, version, etc…

ankon commented 8 years ago

Missed that part. Ok, sounds reasonable (from a gulp-PoV), but from a maven-PoV I find that confusing (proper maven usage means "one artifact per build"). Instead of #16 then, what about logging a warning or outright failing when artifactId is given in the configuration?

micha149 commented 8 years ago

Damn it. You are right. The artifact should be unique for a single deploy. Currently I am struggling around with deploying an artifact wich brings a second artifact which contains the apidoc. Both should be deployed with same artifactId and version but the apidoc gets a classifier. This seems not to be possible with the current implementation :finnadie:

micha149 commented 8 years ago

I created the issue #17 to discuss this problem...