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

#13 - Allow send files after the tag/release be created. #14

Closed
wants to merge 1 commit into from
Closed

#13 - Allow send files after the tag/release be created. #14

wants to merge 1 commit into from

Conversation

weisebrazil
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.469% when pulling e4262e3 on weisebrazil:master into b643f65 on tschulte:master.

@@ -237,6 +242,7 @@ class SemanticReleaseChangeLogService {

Release release = repo.releases().create(tag)
new Release.Smart(release).body(changeLog(commits(Version.valueOf(version.previousVersion)), version).toString())
releaseAsset(release.assets(), "$version.version")
Copy link
Owner

Choose a reason for hiding this comment

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

The second parameter is named currentTag, but the instead the version is passed. The question is, if version and/or tag are necessary at all. The sample code in README.md could also be written as

assets.upload(tasks.jar.outputs.files.singleFile.bytes, "application/zip", tasks.jar.archiveName)

Alternatively, the closure could be named releaseAssets (plural) instead, without parameters, expected to return a List of files. And instead of calling releaseAsset(release.assets()), it would be

releaseAssets().each { release.assets().upload(it.bytes, URLConnection.guessContentTypeFromName(it.name), it.name) }

with

semanticRelease {
    changeLog {
        releaseAssets = {
            tasks.jar.outputs.files
        }
    }
}

But that is less powerful. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Or we could define a member releaseAssets of type FileCollection and create a method releaseAssets(Object):

FileCollection releaseAssets = project.files()

def releaseAssets(Object... assets) {
    releaseAssets += project.files(assets)
}

The above would require some more changes, because the ChangelogService does not know the project yet. But it would allow the user of the plugin to just write

task sourceJar ...
task javadocZip ...
semanticRelease {
    changeLog {
        releaseAssets(jar, sourceJar, javadocZip)
    }
}

because project.files can handle Task instances and automatically uses the task outputs (https://docs.gradle.org/current/dsl/org.gradle.api.Project.html#org.gradle.api.Project:files(java.lang.Object[]))

Copy link
Owner

Choose a reason for hiding this comment

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

The main reason, why I am in favor of the last suggestion: the usage of jcabi-github should be an implementation-detail. That's the reason, why the setter and getter of SemanticReleaseChangeLogService.github were marked as deprecated and will be removed in 2.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to send other kind of files too (.war, .exe), and a way to rename it. About of the file type the method URLConnection.guessContentTypeFromName() don't have all kind of files too.

We can have a default implementation but I don't know the best way to do it.

Copy link
Owner

Choose a reason for hiding this comment

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

You are right. URLConnection.guessContentTypeFromName() doesn't know a lot of file types. E.g. interestingly it does not support jar files.

Do you know, what github uses this information for? I currently have application/octet-stream as fallback.

The possibility to rename an asset or give a description -- which does not seem to be possible with jcabi-github, but is part of the github-api (https://developer.github.com/v3/repos/releases/#upload-a-release-asset) -- is a valid one. With my changes, the only way to achive giving a different name would be to create a copy-task and use that copy instead of the original file.

Thinking about this: by calling project.files() inside the plugin code masks the fact, that this only works for tasks that produce a single file (like Jar and Zip), but not for other tasks (like Copy. By removing this code in the ChangeLogService again and changing the method to void releaseAssets(FileCollection) the code of ChangeLogService would become more clear again, but the user of the plugin would have to write a little more code.

Or there could be some overloaded methods Asset releaseAsset(AbstractArchiveTask), Asset releaseAsset(File) and a new Class Asset with methods Asset contentType(String), Asset name(String) and Asset label(String). This would allow to write

semanticRelease {
    changeLog {
        releaseAsset jar contentType 'application/zip' name 'theapplication.jar' label 'The application'
        // or
        releaseAsset(sourcesJar).contentType("application/zip").name("thesources.jar").label("the sources jar")
    }
}

By changing the methods in the ChangeLogService to Asset releaseAsset(Map params = [:], AbstractArchiveTask task), the user could write releaseAsset sourcesJar, contentType: 'application/zip', name: 'thesources.jar', label: 'the sources jar' in their build file.

@tschulte
Copy link
Owner

I have merged your changes into branch pull-14 and made the changes. At https://github.com/tschulte/gradle-semantic-release-test/releases you can see the first fully automatic release with assets. I will merge these changes back into master tonight and create v1.2.0.

Maybe later this needs some fundamental refactoring. The plugin itself should not be that dependent on github, but is at the moment.

I'm just not sure about commit 8ba51eb (issue #16), which would be included in that version. If everyone used the description in README.md (tasks.release { dependsOn build; finalizedBy publish } this is a non-breaking feature. But if someone used dependsOn publish instead, it would be a breaking feature (thus requiring bumping the version to 2.0.0). What do you think?

Thanks for your contribution.

@tschulte tschulte closed this Aug 11, 2016
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