Add FindStagingRepository task to find staging repository by its description #145

vlsi · 2022-06-30T11:46:52Z

The task would be helpful when "publish staged repository" is implemented as a separate step.

See #19

vlsi · 2022-07-07T11:06:31Z

@szpak , @marcphilipp , any thoughts?

szpak · 2022-07-09T08:16:39Z

@szpak , @marcphilipp , any thoughts?

I'm "out of office" and I will not be able to review that before August :-/.

src/e2eTest/kotlin/io/github/gradlenexus/publishplugin/e2e/NexusPublishE2ETests.kt

vlsi · 2022-09-01T11:53:42Z

@szpak , @marcphilipp , do you think you could have some cycles for reviewing the change?

src/e2eTest/kotlin/io/github/gradlenexus/publishplugin/e2e/NexusPublishE2ETests.kt

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

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

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

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

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

szpak · 2023-02-12T00:54:58Z

Sorry for delay with reviewing @vlsi :-/. I gave it a try today and planned to finish it and merge. However, after I wrote mocked integration tests and I've found some things to discuss (commented inline). Of course, if you are still interested in getting it done.

vlsi · 2023-02-12T20:25:41Z

We should probably define what happens if multiple repositories match.

We might add an enum like: FAIL (default), USE_LATEST

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

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

szpak · 2023-02-19T21:05:22Z

src/e2eTest/kotlin/io/github/gradlenexus/publishplugin/e2e/NexusPublishE2ETests.kt

+
+    @ParameterizedTest(name = "{0}")
+    @ValueSource(strings = ["nexus-publish-e2e-minimal", "nexus-publish-e2e-multi-project"])
+    fun `release project to real Sonatype Nexus in two executions`(projectName: String) {


It will most likely fails, as the version is timestamp based to avoid problem described in [OSSRH-86532] Use different version numbers in e2e on CI #153 (in addition the version is not properly set in the multi-module e2e project as I mentioned in the comment a few days ago).

I can run it from a side branch to verify (if you think 1. is wrong or once there is a workaround - what do you propose?), but in general, I'm still not sure, if we should call it on every merge/push to master, as e2e tests (despite their name) are rather smoke/sanity tests for critical scenarios.

as the version is timestamp based to avoid problem described

What do you think of passing the version on the command line instead?

I'm still not sure, if we should call it on every merge/push to master, as e2e tests (despite their name) are rather smoke/sanity tests for critical scenarios.

There are only 2 merges to master per year. Is adding a test really a problem?
If you absolutely want to reduce the number of interactions with Central, you might consider launching a random subset of tests every time.

There are only 2 merges to master per year. Is adding a test really a problem?

It depends of a time window. I see 17 commits to master in 2 weeks :). With 2x30+ projects releases released to Nexus.

In general, this is not a big problem right now (especially that I plan to reduce number of e2e tests execution #181 to mainly master and cron), but I'm really happy being able to perform the real e2e testing with the real Sonatype Nexus and it would be problematic to get banned (or something). E2E test should cover the main/critical scenarios and from my experience, especially in the "enterprise stuff", they tend to grow (with all the maintenance issues). I'm really curious, how many project will use the new mode (and publishing performed in two phases).

Anyway, let's see how many build it trigger in the coming weeks. In the worst case, we could add a switch to just call them (the new one) from cron, not for every master build.

What do you think of passing the version on the command line instead?

Yes, give me a moment, I'm playing with some idea.

Would adding -PoverriddenVersion=somethingUnique twice with the same (unique) value generated in given be enough?

gradle-nexus/nexus-publish-e2e-minimal@6a6fbdb
gradle-nexus/nexus-publish-e2e-multi-project@a2de98f

(just update the submodules and later commit changes switching the pointers in the main repo in your PR)

Btw, how would you approach the issue with:

:nexus-publish-e2e-multi-project:unspecified

reported for the root project? Here, in the multi-module Kotlin project, the things are slightly complicated as the version is set in buildSrc/library-publishing-conventions.gradle.kts just for submodules?

szpak · 2023-02-20T20:08:12Z

src/e2eTest/kotlin/io/github/gradlenexus/publishplugin/e2e/NexusPublishE2ETests.kt

+
+    @ParameterizedTest(name = "{0}")
+    @ValueSource(strings = ["nexus-publish-e2e-minimal", "nexus-publish-e2e-multi-project"])
+    fun `release project to real Sonatype Nexus in two executions`(projectName: String) {


There are only 2 merges to master per year. Is adding a test really a problem?

It depends of a time window. I see 17 commits to master in 2 weeks :). With 2x30+ projects releases released to Nexus.

In general, this is not a big problem right now (especially that I plan to reduce number of e2e tests execution #181 to mainly master and cron), but I'm really happy being able to perform the real e2e testing with the real Sonatype Nexus and it would be problematic to get banned (or something). E2E test should cover the main/critical scenarios and from my experience, especially in the "enterprise stuff", they tend to grow (with all the maintenance issues). I'm really curious, how many project will use the new mode (and publishing performed in two phases).

Anyway, let's see how many build it trigger in the coming weeks. In the worst case, we could add a switch to just call them (the new one) from cron, not for every master build.

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

szpak · 2023-02-20T20:16:40Z

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

@@ -76,6 +76,16 @@ class NexusPublishPlugin : Plugin<Project> {
                repository,
                registry
            )
+            val findStagingRepository = rootProject.tasks.register<FindStagingRepository>(


Btw, we need to document it somewhere, to let people know. Would you see it in README or we should just mention it in the README, but provide sample used on the wiki?

With -PoverriddenVersion=... However, that version still has to be unique. For e2e tests of releasing in two executions with findStagingRepository: gradle-nexus/publish-plugin#145

vlsi · 2023-02-21T07:34:55Z

After having some more thought, I incline to a different design: what if InitializeNexusStagingRepository was like "find an existing staging repository or create a new one if missing"?

Then it would handle cases like ./gradlew publishToSonatype; ./gradlew closeSonatypeStagingRepository; ./gradlew releaseSonatypeStagingRepository automatically.

Well, it might worth adding an extra property to InitializeNexusStagingRepository to specify the behavior:

FIND_OR_CREATE -- reuses an existing staging repository if present, or creates a new one
CREATE_NEW -- always creates a new staging repository
FIND_OR_FAIL -- searches for an existing repository and fails if the repo is missing
FIND_OR_NULL -- searches for an existing repository, and just keeps null value if the repo is missing

WDYT?

Then:

initialize${repo}StagingRepository would default to FIND_OR_CREATE
find${repo}StagingRepository would default to FIND_OR_FAIL
publish would dependOn(initialize...)
closeStaging... would dependOn(find)
releaseStaging... would dependOn(find)
find would mustRunAfter(initialize)

Then it would be just fine to execute any command individually without mentioning find...StagingRepository explicitly.

:nexus-publish-e2e-multi-project:unspecified

Well, apparently, the staging repository is created after the repository description of the root project. So there are two approaches here:
a) allow overriding the version of the root project (it is the only version that is really needed, and find.. task does not use subproject versions)
b) allow overriding repositoryDescription property in test execution

…ription Fixes #19

vlsi · 2023-02-21T12:57:23Z

I've unified the code and added the description.
With those changes, the publish and close tasks would work without any extra info from the user (there's no need to explicitly pass staging profile id).

szpak · 2023-02-21T21:31:39Z

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

+                            stagingRepositoryId.isPresent -> dummyDetachedConfiguration
+                            else -> findStagingRepositoryTask
+                        }
+                    }


Maybe we could extract it with "meaningful name" and use twice?

E.g.

findStagingRepositoryTaskIfStagingRepositoryIdIsNotPresentProvider(project)

szpak · 2023-02-21T21:57:17Z

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

+
+    @Internal
+    val stagingRepositoryId = objects.property<String>()
+
    @TaskAction
    fun createStagingRepo() {


That name would be no longer valid

szpak · 2023-02-21T22:02:42Z

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

+                        }
+                    }
+                }
+            }


Sorry, but that is too complex as a "one-liner" assigning a value to descriptor. 5 levels of nested statement and clumsy conditional logic. It would need to be refactored into smaller pieces, however, please take a look at the "general comment" to new approach before doing any actions here.

szpak · 2023-02-21T22:03:06Z

In general, changing the behavior of the init task (and effectively also publishToSonatype) is not a backward compatible change. I incline to postpone it to the next release to avoid confusion from people upgrading "just to fix deprecation warning".

Talking about the feature itself, I might imagine a situation when, someone calls publishToSonatype and closeSonatypeStagingRepository is the close operation fails (due to some formal validation issues). He fixes the problem locally and call publishToSonatype again. The existing repo is found and the files are added to it (it wasn't closed due to the error). It's rather not the expected situation (and as I understood the latest code, it would behave that way).

I wonder if switching initialize to CREATE_NEW would "fix" the problem, it there are some corner cases? However, in that case (CREATE_NEW), I'm not sure if it still makes sense to merge the new logic into the InitializeNexusStagingRepository .

vlsi · 2023-02-21T22:21:15Z

I am afraid we go in circles, and there's no way it could be merged. I would rather close the pr and fork gnpp

szpak · 2023-02-21T22:41:21Z

I am afraid we go in circles, and there's no way it could be merged. I would rather close the pr and fork gnpp

Sorry @vlsi, but this time I really don't get you.

Please take a look at the open comment yesterday:

broken e2e tests - I "fixed" them partially and asked you about the idea how to fix the other part (the root project)
a need for documentation

Otherwise, I liked the version and wanted to merge it (with working e2e tests and docs). Are those two comments above ridiculous?

And today, suddenly, you completely (really) reworked the whole concept. I put the two main comments:

a common case which breaks the backward compatibility and - in my opinion - introduces a behavior that is not expected by the majority of people and ask for your comment:

Talking about the feature itself, I might imagine a situation when, someone calls publishToSonatype and closeSonatypeStagingRepository is the close operation fails (due to some formal validation issues). He fixes the problem locally and call publishToSonatype again. The existing repo is found and the files are added to it (it wasn't closed due to the error). It's rather not the expected situation (and as I understood the latest code, it would behave that way).

one too complex part of code - and here, sorry to say, but for me 5 levels of nested statements with complex logic inside was hard to read (maybe I'm just a Kotlin noob) and I proposed extracting it to smaller methods:

I believe that for also for some other people reading that code it could be "not so straightforward" in that form.

If that "is getting nowhere", well, I don't know what to response.

vlsi · 2023-02-22T08:56:07Z

What bothers me the most is:

There's virtually no feedback on the high level
- Add FindStagingRepository task to find staging repository by its description #145 (comment)
- Replace submodules with regular folders under publish-plugin repository #182 -- currently there's no way to execute tests, and there's no clarification why a part of the test code is moved to a different repository.
There are a lot of comments on the style side like "extracting variables", "too long lines", "method names"

My expectations are:

It is fine to comment on the code style in case the overall concept is ok
It is not fine to ignore the high level and comment on variable names
It is not fine to have "this breaks backward compatibility, and this variable has a bad name" at the same time

What I see is pure unpredictability. I have absolutely no idea what could be merged and what not.
I do not want to spend my time trying to figure out what pleases the maintainers of the project.

Of course, you are free to do whatever you want with the project, however, based on the reviews above I lost interest in contributing further.

szpak · 2023-02-22T19:18:43Z

There's virtually no feedback on the high level

#145 (comment)

You closed that one and in the other thread I proposed to generate a unique version in Gradle and call it twice in the e2e test (at least for single-module project it should work right now).

#182 -- currently there's no way to execute tests

That's not true. They are separate projects. You can clone it and release with a regular Gradle command from your console. Just provide your own staging profile and credentials. There were separated to have that freedom.
Similar approach (with own credentials) should be possible also to call them locally from e2e tests. Submodules can make it slightly less usable and we could consider their incorporation into the main repo, but it is not a blocker.

Having them (just the one with the single module, unless you already fixed the issue with the multi-module one) green locally (or feeling it should be), I can trigger them manually from CI, after the recent changes - #183. Neverhtless, if you feel overwhelmed by the submodules, I can do that myself in this branch. No problem. Just let me know.

szpak · 2023-02-22T19:29:11Z

Of course, you are free to do whatever you want with the project, however, based on the reviews above I lost interest in contributing further.

I see and regret. You were a valuable contributor. In that case, I will probably take the previous version of this PR, which - as I wrote - is fine for me to merge (the one before you unexpectedly proposed revolutionary changes, which I had some doubts about and suggested an important case which could be broken, but you didn't even try to comment it), add one e2e test, write some documentation to allow people to find that feature and put it into the next release.

Thanks for all your contribution (in code and comments) so far (in that project and many others!). Maybe you will find this project interesting to contribution in the future again.

szpak · 2023-02-26T13:21:26Z

Superseded by #201.

vlsi commented Sep 1, 2022

View reviewed changes

src/e2eTest/kotlin/io/github/gradlenexus/publishplugin/e2e/NexusPublishE2ETests.kt Outdated Show resolved Hide resolved

vlsi mentioned this pull request Sep 14, 2022

Question: Publish and Closing in a Different Step #134

Open

vlsi mentioned this pull request Feb 5, 2023

Add more committers/releasers #156

Closed

szpak requested changes Feb 12, 2023

View reviewed changes

szpak reviewed Feb 12, 2023

View reviewed changes

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

szpak added this to the 1.2.0 milestone Feb 17, 2023

vlsi requested a review from szpak February 19, 2023 20:09

szpak requested changes Feb 19, 2023

View reviewed changes

vlsi mentioned this pull request Feb 19, 2023

Replace submodules with regular folders under publish-plugin repository #182

Open

szpak reviewed Feb 20, 2023

View reviewed changes

Add FindStagingRepository task to find staging repository by its desc…

71803c6

…ription Fixes #19

Unify code for FindStagingRepository and InitializeStagingRepository

6fe247d

szpak requested changes Feb 21, 2023

View reviewed changes

szpak modified the milestones: 1.2.0, 2.0.0 Feb 22, 2023

vlsi closed this Feb 22, 2023

vlsi deleted the findStagingRepository branch February 22, 2023 19:49

This was referenced Feb 26, 2023

Improve root project version in nexus-publish-e2e-multi-project #200

Open

Add FindStagingRepository task to find staging repository by its description #201

Merged

szpak removed this from the 2.0.0 milestone Feb 26, 2023

vlsi mentioned this pull request Jun 24, 2023

[stage-vote-release-plugin] Execution failed for task ':releaseRepository' due to missing nexus.txt file vlsi/vlsi-release-plugins#64

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FindStagingRepository task to find staging repository by its description #145

Add FindStagingRepository task to find staging repository by its description #145

vlsi commented Jun 30, 2022

vlsi commented Jul 7, 2022

szpak commented Jul 9, 2022

vlsi commented Sep 1, 2022

szpak commented Feb 12, 2023 •

edited

Loading

vlsi commented Feb 12, 2023

szpak Feb 19, 2023

vlsi Feb 19, 2023

szpak Feb 20, 2023

szpak Feb 20, 2023

szpak Feb 20, 2023 •

edited

Loading

szpak Feb 20, 2023

szpak Feb 20, 2023

szpak Feb 20, 2023

vlsi commented Feb 21, 2023 •

edited

Loading

vlsi commented Feb 21, 2023

szpak Feb 21, 2023

szpak Feb 21, 2023

szpak Feb 21, 2023

szpak commented Feb 21, 2023

vlsi commented Feb 21, 2023 •

edited

Loading

szpak commented Feb 21, 2023 •

edited

Loading

vlsi commented Feb 22, 2023

szpak commented Feb 22, 2023 •

edited

Loading

szpak commented Feb 22, 2023 •

edited

Loading

szpak commented Feb 26, 2023

Add FindStagingRepository task to find staging repository by its description #145

Add FindStagingRepository task to find staging repository by its description #145

Conversation

vlsi commented Jun 30, 2022

vlsi commented Jul 7, 2022

szpak commented Jul 9, 2022

vlsi commented Sep 1, 2022

szpak commented Feb 12, 2023 • edited Loading

vlsi commented Feb 12, 2023

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 Feb 20, 2023 • 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

vlsi commented Feb 21, 2023 • edited Loading

vlsi commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szpak commented Feb 21, 2023

vlsi commented Feb 21, 2023 • edited Loading

szpak commented Feb 21, 2023 • edited Loading

vlsi commented Feb 22, 2023

szpak commented Feb 22, 2023 • edited Loading

szpak commented Feb 22, 2023 • edited Loading

szpak commented Feb 26, 2023

szpak commented Feb 12, 2023 •

edited

Loading

szpak Feb 20, 2023 •

edited

Loading

vlsi commented Feb 21, 2023 •

edited

Loading

vlsi commented Feb 21, 2023 •

edited

Loading

szpak commented Feb 21, 2023 •

edited

Loading

szpak commented Feb 22, 2023 •

edited

Loading

szpak commented Feb 22, 2023 •

edited

Loading