[#4][#6] Close and release tasks #9

szpak · 2019-10-18T17:30:01Z

The initial implementation of close and release tasks. It's effectively broken with the real Nexus instance (due to missing retrier to handle "in transition" periods - #7), but I wanted to cover it in another PR as I had some issue with Kotlin and I'm not sure about some Gradle related bindings here. Therefore, I would like to get faster feedback for the following code.

In the next PR I plan to work on:

retrier to cover "inTransition == true" case
E2E tests
duplication reduction in tests
removing GNSP reference

szpak · 2019-10-18T17:31:26Z

src/main/kotlin/io/github/gradlenexus/publishplugin/InitializeNexusStagingRepository.kt

-        connectTimeout.set(extension.connectTimeout)
-        this.onlyIf { extension.useStaging.getOrElse(false) }
-    }
+    private val stagingRepositoryId: Property<String> = repository.stagingRepositoryId


I'm not sure about the best way to bind stagingRepositoryId in InitializeNexusStagingRepository and Repository.stagingRepositoryId, to be able to set value in task to set it in Extension/Repository. I initially just set stagingRepositoryId: Property<String> = repository.stagingRepositoryId, but I don't like sharing a reference, so maybe you have any better idea.

= objects.property().convention(repository.stagingRepositoryId) ?

Hmm, Unless I don't understand convention() properly, I will not be able to propagate value back to repository once it is set in the task. I need the other way round connection: task -> repository.

Oh, now I understand your intention.

Can you please clarify how do you mean the task should be used?
I think the user does not know stagingRepositoryId before the task is executed, so I don't see how the user would configure the property.

User could override the property in extension.repository to for example close or releate a requested staging repository with id known from the previous (failed) closing operation.

How problematic would it be to do the clean up in the second call? E.g.

gw release* -PyourID* && gw dropAllRepositories*

(assuming there will be that task one day)

While technically it is possible to call close/release/drop commands in Nexus with multiple repository IDs it's not a case with simple init/publish/close/release use case. On the other hand I can't find any sensible obstacles to use ListProperty<String> in the extension and have init to set just one element (there could be also some helper method to set just one).

How problematic would it be to do the clean up in the second call?

Users are not always comfortable with storing passwords/keys, so "release / drop" might require the user to type the password (or provide it with environment/property).
So your example with && would be much more complicated, as there are multiple credentials involved. In Apache case the credentials are for dist.apache.org (SVN), repository.apache.org (Nexus), gitbox.apache.org (Git) all of which participate in the release candidate/release publish.

Having a single command helps there.

Then there's "error recovery". If the actions are split into multiple commands, it makes it more complicated to "resume after failure". It looks like the user would have to analyze which command failed in order to retry or rollback.

Why do we need this property in this class at all? Wouldn't it suffice to set it in NexusRepository? In addition, I think we should write it to a file and read it from there in NexusRepository by default. That way, one could initialize and publish in the first Gradle invocation, do some manual testing, and close and release without passing the stagingRepositoryId manually. Thought?

Currently NexusRepository is only passed to the constructor to collect some required values. The reference is not kept. However, if you are fine with that I will refactor the code.

Regarding keeping it in the file I propose to postpone it to Milestone 2 - it's not MVP.

szpak · 2019-10-18T17:32:08Z

src/test/kotlin/io/github/gradlenexus/publishplugin/TaskOrchestrationTest.kt

+    @Disabled("Broken - must run only after publishMavenJavaPublicationToSonatypeRepository not publishToSonatypeRepository")
+    internal fun `close task should run after related publish`() {
+        initSingleProjectWithDefaultConfiguration()
+        assertGivenTaskMustRunAfterAnother("closeSonatypeStagingRepository", "publishToSonatypeRepository")


publishToNexusTask (and close/release tasks) depends/mustRunAfter publishMavenJavaPublicationToSonatypeRepository. It would be probably better to use publishToSonatypeRepository to also cover PDF documentation update, CDC stubs, etc. @marcphilipp How would be tweak it to cover that?

I'm afraid I don't understand your question. Could you please elaborate a bit more?

Sure. How the easiest would be to change the plugin implementation to make closeSonatypeStagingRepository depends on publishToSonatypeRepository instead of all the related publications separately (e.g. publishMavenJavaPublicationToSonatypeRepository, publishPdfDocumentationPublicationToSonatypeRepository)?

It's not a big deal though as a publication for PDF documents should be also covered by:

val publishTasks = project.tasks .withType<PublishToMavenRepository>() .matching { it.repository == nexusRepository }

Can we just add closeTask.mustRunAfter(publishToNexusTask) and the same for releaseTask?

How would be best to get a reference/provider to the umbrella publish task in the production plugin code?

szpak · 2019-10-18T17:35:05Z

Hmm, is it possible to switch to "draft PR" once a PR is created?

Initial version.

src/main/kotlin/io/github/gradlenexus/publishplugin/InitializeNexusStagingRepository.kt

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusRepository.kt

src/main/kotlin/io/github/gradlenexus/publishplugin/ReleaseNexusStagingRepository.kt

vlsi · 2019-10-19T06:13:01Z

src/main/kotlin/io/github/gradlenexus/publishplugin/internal/NexusClient.kt

@@ -97,6 +97,28 @@ class NexusClient(private val baseUrl: URI, username: String?, password: String?
        }
    }

+    fun closeStagingRepository(stagingRepositoryId: String) {
+        try {
+            val response = api.closeStagingRepo(Dto(StagingRepositoryToTransit(listOf(stagingRepositoryId), "Closed by io.github.gradle-nexus.publish-plugin Gradle plugin"))).execute()


I wish there was a more-or-less meaningful (configurable?) comment.

Please raise an issue for that. Could be implemented in the future - not an MVP.

vlsi · 2019-10-19T06:13:39Z

src/main/kotlin/io/github/gradlenexus/publishplugin/internal/NexusClient.kt

+        try {
+            val response = api.closeStagingRepo(Dto(StagingRepositoryToTransit(listOf(stagingRepositoryId), "Closed by io.github.gradle-nexus.publish-plugin Gradle plugin"))).execute()
+            if (!response.isSuccessful) {
+                throw failure("close staging repository", response)


Should failure message include the repository id / group id?

I plan to make error handling for general for all the calls.

src/main/kotlin/io/github/gradlenexus/publishplugin/internal/NexusClient.kt

marcphilipp

I didn't have time for a full review, yet, but added some comments. I'll continue tomorrow. 🙂

build.gradle.kts

src/compatTest/kotlin/io/github/gradlenexus/publishplugin/NexusPublishPluginTests.kt

src/main/kotlin/io/github/gradlenexus/publishplugin/BaseOperationOnNexusStagingRepository.kt

marcphilipp · 2019-10-23T18:09:31Z

src/main/kotlin/io/github/gradlenexus/publishplugin/BaseOperationOnNexusStagingRepository.kt

+    protected val clientTimeout: Property<Duration> = objects.property()
+
+    @get:Internal
+    protected val connectTimeout: Property<Duration> = objects.property()


❓ Maybe we should extract these properties into a @Nested NexusConfiguration object instead of using a base class?

One could want to have a separated timeout per repository. However, there still should be an easy way to set it globally (as in most of the cases it is enough - in fact most of the users (except @vlsi ;) ) will only use one repository per build).

src/main/kotlin/io/github/gradlenexus/publishplugin/CloseNexusStagingRepository.kt

marcphilipp · 2019-11-01T19:22:20Z

src/test/kotlin/io/github/gradlenexus/publishplugin/TaskOrchestrationTest.kt

+    @Disabled("Broken - must run only after publishMavenJavaPublicationToSonatypeRepository not publishToSonatypeRepository")
+    internal fun `close task should run after related publish`() {
+        initSingleProjectWithDefaultConfiguration()
+        assertGivenTaskMustRunAfterAnother("closeSonatypeStagingRepository", "publishToSonatypeRepository")


I'm afraid I don't understand your question. Could you please elaborate a bit more?

marcphilipp · 2019-11-01T19:29:12Z

src/main/kotlin/io/github/gradlenexus/publishplugin/InitializeNexusStagingRepository.kt

-        connectTimeout.set(extension.connectTimeout)
-        this.onlyIf { extension.useStaging.getOrElse(false) }
-    }
+    private val stagingRepositoryId: Property<String> = repository.stagingRepositoryId


Why do we need this property in this class at all? Wouldn't it suffice to set it in NexusRepository? In addition, I think we should write it to a file and read it from there in NexusRepository by default. That way, one could initialize and publish in the first Gradle invocation, do some manual testing, and close and release without passing the stagingRepositoryId manually. Thought?

By keeping a reference to NexusRepository.

See: diffplug/spotless#476 As a side effect import-ordering has been disabled to do not break the build. It changed to fail after upgrade. I don't want to mix that PR with global import reordering.

szpak · 2019-11-06T17:01:04Z

src/main/kotlin/io/github/gradlenexus/publishplugin/AbstractNexusStagingRepositoryTask.kt

+    @get:Internal
+    protected val connectTimeout: Property<Duration> = objects.property()
+
+    private val repository: Property<NexusRepository> = objects.property()


@marcphilipp Have you thought about something like to that to keep NexusRepository in a task? Our previous conversation disappeared :-/

Well, we could use @Nested and remove all the fields that just copy its properties. Doing both looks redundant to me.

Using a command line switch.

szpak · 2020-01-07T18:00:25Z

src/main/kotlin/io/github/gradlenexus/publishplugin/CloseNexusStagingRepository.kt

+        val client = NexusClient(repository.get().nexusUrl.get(), repository.get().username.orNull, repository.get().password.orNull, clientTimeout.orNull, connectTimeout.orNull)
+        val stagingProfileId = determineAndCacheStagingProfileId(client)
+        logger.info("Closing staging repository with id '{}' for stagingProfileId '{}'", stagingRepositoryConfig.get().repositoryId, stagingProfileId)
+        client.closeStagingRepository(stagingRepositoryConfig.get().repositoryId!!) //should be checked by @Input


I don't feel comfortable with using !! and mutable structures (with nullable fields) at all. However, those 2 properties can be set independently and in addition they can be null (in some time periods - set by the earlier task execution or overridden with @Option). Nevertheless, I would like to hear any constructive proposal how to deal with it in some "better way".

- Remove stagingProfileId from close/release tasks as it is not needed - Wire stagingRepositoryId for close/release task in plugin - Remove mutable state from NexusRepository - Rename CLI option to --staging-repository-id to make it more idiomatic

szpak · 2020-01-12T01:21:15Z

Applied @marcphilipp changes from #11.

szpak · 2020-01-12T01:51:14Z

Merging conflicts due to the recent changes (spotless upgrade and import reordering) in master :-/. Will fix.

marcphilipp · 2020-01-12T13:28:18Z

Sorry! 😬

marcphilipp · 2020-01-12T13:29:58Z

src/main/kotlin/io/github/gradlenexus/publishplugin/InitializeNexusStagingRepository.kt

@@ -40,7 +39,6 @@ constructor(
    private val stagingRepositoryId: (String) -> Unit
 ) : AbstractNexusStagingRepositoryTask(objects, extension, repository) {

-    @Optional


It's only required if stagingProfileId is not set. Thus, I think @Optional was correct, wasn't it?

Well, they both can be not set and then there will be val packageGroup = packageGroup.get() called. However, it would be problematic to add two-field present validation, so probably packageGroup.get() is good enough, especially as it reports the name of the not provided property. I will restore @Optional.

[#4] Base class for staging repository operations

b92668a

szpak commented Oct 18, 2019

View reviewed changes

[#4] Close and release tasks

48db8ad

Initial version.

szpak force-pushed the closeAndRelease branch from 27dc786 to 48db8ad Compare October 18, 2019 17:37

marcphilipp and others added 2 commits October 18, 2019 19:43

Use Kotlin DSL extensions

225637e

Apply also java plugin in tests

50a46d7

vlsi reviewed Oct 19, 2019

View reviewed changes

src/main/kotlin/io/github/gradlenexus/publishplugin/InitializeNexusStagingRepository.kt Outdated Show resolved Hide resolved

vlsi reviewed Oct 19, 2019

View reviewed changes

src/main/kotlin/io/github/gradlenexus/publishplugin/NexusRepository.kt Outdated Show resolved Hide resolved

vlsi reviewed Oct 19, 2019

View reviewed changes

src/main/kotlin/io/github/gradlenexus/publishplugin/ReleaseNexusStagingRepository.kt Outdated Show resolved Hide resolved

vlsi reviewed Oct 19, 2019

View reviewed changes

src/main/kotlin/io/github/gradlenexus/publishplugin/internal/NexusClient.kt Outdated Show resolved Hide resolved

szpak added 2 commits October 19, 2019 16:11

[#4] Simplify IO error handling

a958502

[#4] Review suggestions

00d875e

marcphilipp reviewed Oct 23, 2019

View reviewed changes

marcphilipp self-requested a review October 23, 2019 18:10

marcphilipp reviewed Nov 1, 2019

View reviewed changes

szpak added 3 commits November 4, 2019 21:00

[#4] Review suggestions

afbb6f4

[#4] Rename base task class

05b4e81

[#4] Simplify InitializeNexusStagingRepository

2f10cfc

By keeping a reference to NexusRepository.

szpak requested a review from marcphilipp November 4, 2019 21:20

marcphilipp and others added 4 commits November 5, 2019 20:42

Merge remote-tracking branch 'origin/master' into closeAndRelease

3745ef1

[#4] @option for stagingRepositoryId in close and release

6f2c93c

Bump spotless version to effectively enable disabled_rules

ef5a14f

See: diffplug/spotless#476 As a side effect import-ordering has been disabled to do not break the build. It changed to fail after upgrade. I don't want to mix that PR with global import reordering.

[#4] Fix issue with imports

092b317

szpak commented Nov 6, 2019

View reviewed changes

szpak added 2 commits December 27, 2019 15:25

[#4] Reuse common properties from NexUsRepository in Task

2531bac

[#4] Workaround problem with finalized properties in task

716a6b1

[#4] Simplify NexusStagingRepository class

edb31ee

szpak force-pushed the closeAndRelease branch from a22e537 to edb31ee Compare January 3, 2020 15:35

szpak added 2 commits January 4, 2020 14:01

[#4] Override staging repo id to close/release

7388b13

Using a command line switch.

[#4] Determine and cache staging profile id between tasks

084a51a

szpak force-pushed the closeAndRelease branch from 473a97f to 084a51a Compare January 7, 2020 17:57

szpak commented Jan 8, 2020

View reviewed changes

Don't share state via NexusRepository

6ce5c2b

- Remove stagingProfileId from close/release tasks as it is not needed - Wire stagingRepositoryId for close/release task in plugin - Remove mutable state from NexusRepository - Rename CLI option to --staging-repository-id to make it more idiomatic

Merge branch 'master' into closeAndRelease

5b2c8ce

marcphilipp reviewed Jan 12, 2020

View reviewed changes

szpak force-pushed the closeAndRelease branch from 3dc3036 to 5b2c8ce Compare January 12, 2020 14:12

marcphilipp approved these changes Jan 15, 2020

View reviewed changes

szpak merged commit 5b2c8ce into master Jan 23, 2020

szpak changed the title ~~[WIP] [#4][#6] Close and release tasks~~ [#4][#6] Close and release tasks Feb 5, 2020

szpak mentioned this pull request Mar 12, 2020

Add task for releasing a staging repository #6

Closed

marcphilipp added this to the 0.1.0 milestone May 31, 2020

marcphilipp deleted the closeAndRelease branch November 14, 2021 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4][#6] Close and release tasks #9

[#4][#6] Close and release tasks #9

szpak commented Oct 18, 2019

szpak Oct 18, 2019

vlsi Oct 19, 2019

szpak Oct 19, 2019

vlsi Oct 20, 2019

szpak Oct 20, 2019

szpak Oct 20, 2019 •

edited

Loading

vlsi Oct 21, 2019

marcphilipp Nov 1, 2019

szpak Nov 4, 2019

szpak Nov 4, 2019

szpak Oct 18, 2019

marcphilipp Nov 1, 2019

szpak Nov 4, 2019 •

edited

Loading

marcphilipp Nov 5, 2019

szpak Nov 5, 2019

szpak commented Oct 18, 2019 •

edited

Loading

vlsi Oct 19, 2019

szpak Oct 19, 2019

vlsi Oct 19, 2019

szpak Oct 19, 2019

marcphilipp left a comment

marcphilipp Oct 23, 2019

szpak Nov 4, 2019

marcphilipp Nov 1, 2019

marcphilipp Nov 1, 2019

szpak Nov 6, 2019

marcphilipp Nov 20, 2019

szpak Jan 7, 2020

szpak commented Jan 12, 2020

szpak commented Jan 12, 2020

marcphilipp commented Jan 12, 2020

marcphilipp Jan 12, 2020

szpak Jan 12, 2020

[#4][#6] Close and release tasks #9

[#4][#6] Close and release tasks #9

Conversation

szpak commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szpak Oct 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szpak Nov 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szpak commented Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcphilipp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szpak commented Jan 12, 2020

szpak commented Jan 12, 2020

marcphilipp commented Jan 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szpak Oct 20, 2019 •

edited

Loading

szpak Nov 4, 2019 •

edited

Loading

szpak commented Oct 18, 2019 •

edited

Loading