publish-plugin icon indicating copy to clipboard operation
publish-plugin copied to clipboard

Add FindStagingRepository task to find staging repository by its description

Open vlsi opened this issue 2 years ago • 3 comments

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

See #19

vlsi avatar Jun 30 '22 11:06 vlsi

@szpak , @marcphilipp , any thoughts?

vlsi avatar Jul 07 '22 11:07 vlsi

@szpak , @marcphilipp , any thoughts?

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

szpak avatar Jul 09 '22 08:07 szpak

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

vlsi avatar Sep 01 '22 11:09 vlsi

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.

szpak avatar Feb 12 '23 00:02 szpak

We should probably define what happens if multiple repositories match.

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

vlsi avatar Feb 12 '23 20:02 vlsi

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

vlsi avatar Feb 21 '23 07:02 vlsi

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).

vlsi avatar Feb 21 '23 12:02 vlsi

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 .

szpak avatar Feb 21 '23 22:02 szpak

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

vlsi avatar Feb 21 '23 22:02 vlsi

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: image

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.

szpak avatar Feb 21 '23 22:02 szpak

What bothers me the most is:

  • There's virtually no feedback on the high level
    • https://github.com/gradle-nexus/publish-plugin/pull/145#discussion_r1103829334
    • https://github.com/gradle-nexus/publish-plugin/issues/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.

vlsi avatar Feb 22 '23 08:02 vlsi

There's virtually no feedback on the high level

https://github.com/gradle-nexus/publish-plugin/pull/145#discussion_r1103829334

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).

https://github.com/gradle-nexus/publish-plugin/issues/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 avatar Feb 22 '23 19:02 szpak

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 avatar Feb 22 '23 19:02 szpak

Superseded by #201.

szpak avatar Feb 26 '23 13:02 szpak