Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to specify targets and push to multiple maven repos. #23

Merged
merged 5 commits into from
Oct 6, 2018
Merged

Add the ability to specify targets and push to multiple maven repos. #23

merged 5 commits into from
Oct 6, 2018

Conversation

budius
Copy link
Contributor

@budius budius commented Sep 25, 2018

The client code will simply create a mavenRepositories{ } and add info on the needed repos.
Example:

mavenRepositories {
	internalRepo = {
                releaseRepositoryUrl = MAVEN_URL_INTERNAL
                snapshotRepositoryUrl = MAVEN_URL_SNAPSHOT_INTERNAL
                repositoryUsername = MAVEN_USERNAME_INTERNAL
                repositoryPassword = MAVEN_PASSWORD_INTERNAL
	}
	betaRepo = {
                releaseRepositoryUrl = MAVEN_URL_BETA
                snapshotRepositoryUrl = MAVEN_URL_SNAPSHOT_BETA
                repositoryUsername = MAVEN_USERNAME_BETA
                repositoryPassword = MAVEN_PASSWORD_BETA
	}
}

and this will create the tasks uploadInternalRepo and uploadBetaRepo

The client code will simply create a `mavenRepositories{ }` and add info on the needed repos.
Example:

mavenRepositories {
	internalRepo = {
                releaseRepositoryUrl = MAVEN_URL_INTERNAL
                snapshotRepositoryUrl = MAVEN_URL_SNAPSHOT_INTERNAL
                repositoryUsername = MAVEN_USERNAME_INTERNAL
                repositoryPassword = MAVEN_PASSWORD_INTERNAL
	}
	betaRepo = {
                releaseRepositoryUrl = MAVEN_URL_BETA
                snapshotRepositoryUrl = MAVEN_URL_SNAPSHOT_BETA
                repositoryUsername = MAVEN_USERNAME_BETA
                repositoryPassword = MAVEN_PASSWORD_BETA
	}
}

and this will create the tasks `uploadInternalRepo` and `uploadBetaRepo`
@budius budius mentioned this pull request Sep 25, 2018
@codecov
Copy link

codecov bot commented Sep 25, 2018

Codecov Report

Merging #23 into master will decrease coverage by 3.68%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #23      +/-   ##
=========================================
- Coverage     93.68%   90%   -3.69%     
- Complexity        7    13       +6     
=========================================
  Files             2     3       +1     
  Lines            95   120      +25     
  Branches          6    14       +8     
=========================================
+ Hits             89   108      +19     
- Misses            1     5       +4     
- Partials          5     7       +2
Impacted Files Coverage Δ Complexity Δ
...ktech/maven/publish/MavenPublishPluginExtension.kt 70.58% <68.75%> (+50.58%) 4 <1> (-1) ⬇️
...vanniktech/maven/publish/MavenPublishPlugin.groovy 93.75% <85.18%> (-4.03%) 6 <5> (+4)
...com/vanniktech/maven/publish/MavenPublishTarget.kt 85.71% <85.71%> (ø) 3 <3> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c69001e...cb93c48. Read the comment docs.

@gabrielittner
Copy link
Collaborator

I think it would be great if more of the code could be reused. The new MavenPublishRepositoryModel and the existing extension have exactly the same properties. The mavenDeployer configuration is the same except where they get their values from.

@budius
Copy link
Contributor Author

budius commented Sep 25, 2018

@gabrielittner I totally agree it's not the best reusable code. Now I'm slightly under-pressure to get moving with the project and got it done just to the point of having the minimum working.

It will take some time until I'll be able to come back to it and get a refactor to unify it all. If anyone feel like taking a stab at it before I come back, go for it.

But for a clean re-use possibly would need to change the plugin API.
What I mean is, instead of mavenPublish { with the 4 parameters and then another mavenRepositories { with a map of the parameters, we could unify them.

My first suggestion would be like

mavenPublish {
   archives {
       // those will be used in `uploadArchives` and automatically grab the default as in MavenPublishPluginExtension.kt
    }
   // others, those need to explicitly declared, we won't check `findProperty` or `enviroment`

but then I'll certainly wait for the maintainers to give a green light before changing.

@gabrielittner
Copy link
Collaborator

gabrielittner commented Sep 25, 2018

I think it would already be possible to reuse some of the plugin code without breaking the API.

If we have an interface like the following both the existing extension and your new model could implement it and then we could have a shared method that does this part

repositories {
mavenDeployer {
beforeDeployment { MavenDeployment deployment -> project.signing.signPom(deployment) }
repository(url: extension.releaseRepositoryUrl) {
authentication(userName: extension.repositoryUsername, password: extension.repositoryPassword)
}
snapshotRepository(url: extension.snapshotRepositoryUrl) {
authentication(userName: extension.repositoryUsername, password: extension.repositoryPassword)
}
configurePom(p, pom)
}
}

interface RepositoryModel {
  var releaseRepositoryUrl: String
  var snapshotRepositoryUrl: String
  var repositoryUsername: String?
  var repositoryPassword: String?
}

I'd be happy to do a follow up on this if you're too busy right now, but let's wait for what @vanniktech thinks.

@vanniktech
Copy link
Owner

Sorry for answering late to this.

Yes, we should reuse as much as possible. Also, the task names should be prefixed with uploadArchives and not only with upload.

Feel free to improve this @gabrielittner and create a PR against this one so we'll ship this feature in one go (to not break snapshot users - if there are any).

Also, the default of the new behavior should be exactly the same as it was before. So configuring this is optional and will just extend the functionality. The normal uploadArchives and installArchives should behave exactly like before.

#9 - should not break either. On a side note, can't we use this mechanism somehow smartly?

@budius
Copy link
Contributor Author

budius commented Sep 25, 2018

@gabrielittner I like it, and it makes sense to me. I gotta admit I had too much fun playing with gradle plugin and all the power that it brings so I'll try to squeeze an hour tomorrow.

@vanniktech prefix with uploadArchives sounds reasonable, can change that easy/fast enough.

Regarding this:

the default of the new behavior should be exactly the same as it was before

Do you mean on the MavenPublishPluginExtension auto picking some defaults from the findProperty or enviroment?

In terms of change, it would make simpler to code, but I'm not sure I agree with the behavior. Reason is, you have a default task uploadArchive that uses default parameters RELEASE_REPOSITORY_URL, etc, etc. And then separately there's the non default tasks with non default parameters. Seems to me like it's easy for a client application to add the duplicate parameter name if it really needs to, than having some default param in background and risk send wrong archive to wrong repo.

Regarding #9,if I understood the concern correctly, I've been using this method here findProperty(String key, String defaultValue) and it works really nice, I could just bring it over to this project and apply throughout.

Also I'll leave a question about unifying into 1 API, or keep 2 separate. Which one?

@budius
Copy link
Contributor Author

budius commented Sep 25, 2018

oh, never mind, I just saw this #24 superseding my PR.

@budius
Copy link
Contributor Author

budius commented Sep 27, 2018

This got branched and improved on #25

@budius budius closed this Sep 27, 2018
@budius
Copy link
Contributor Author

budius commented Sep 27, 2018

Actually, roll back

@budius budius reopened this Sep 27, 2018
@budius
Copy link
Contributor Author

budius commented Sep 27, 2018

updated PR with commits from @gabrielittner

I have the changes locally and will update the README once 0.6.0 is released. Once this is done we can update the open PR and merge it.
@vanniktech
Copy link
Owner

@budius feel free to update this PR once more and we can merge this

@budius
Copy link
Contributor Author

budius commented Oct 4, 2018

@vanniktech sorry I took too long. Had a few days off.
Updated.

@vanniktech vanniktech changed the title Added the hability to configure multiple maven repos. Add the ability to specify targets and push to multiple maven repos. Oct 6, 2018
Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

No rush. Thanks for all of the work!

@vanniktech vanniktech merged commit 4b908e6 into vanniktech:master Oct 6, 2018
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.

3 participants