Repository navigation
build: publish javaish packages to wire maven - #2683
coriolinus wants to merge 8 commits into
Conversation
We want to publish our java-ish artifacts to an internal maven instance, but there are a couple of rules we want to follow which make it complicated to do in bash. In particular: - exclude pre-releases from the metadata but still upload them - execute verification and upload as separate steps - validate signatures Where bash falls short, there is python. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
May as well ensure those always build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mainly these are about coordinating the required inputs in a GHA- appropriate format, and then handing things off to the python script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- added the `wireMaven` repository - removed the nexus plugin and related chaff - removed old gradle properties relating to the vanniktech gradle-maven-publish plugin, which we haven't used for some time Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Particularly relevant to kotlin devs who need to change the repository at which they fetch CC. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![semgrep-code-wireapp[bot]](https://avatars.githubusercontent.com/u/16047324?s=60&v=4){kind=small}
|
For reviewers: f0c1aef just runs |
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(pytest.main([__file__, *sys.argv[1:]])) |
There was a problem hiding this comment.
I’m concerned about the size of the test suite being introduced here relative to the complexity and scope of the utility script. While the intent is understandable, this feels disproportionate to the code being tested. The tests themselves are becoming a significant amount of code to maintain compared with the implementation they are validating.
There’s also a maintenance cost to consider. Any future changes to the script’s behavior or inputs could require updating a large number of tests, even when the underlying change is relatively minor. This adds friction to what should otherwise remain a small and straightforward utility.
I’m also not convinced that the additional coverage provides enough value to justify that cost. A large number of highly specific tests can give us a sense of completeness without necessarily providing proportional confidence in the real-world behavior of the script. In particular, tests that closely mirror implementation details can become brittle and make otherwise safe refactoring unnecessarily difficult.
Given the relatively limited scope and complexity of this utility, I think this PR is over-investing in test coverage. The amount of test code and the ongoing maintenance burden seem out of proportion to the risk addressed by these tests.
I’d therefore prefer not to introduce this large test suite as part of this change. Keeping the utility lightweight also means keeping its surrounding maintenance and development overhead lightweight.
There was a problem hiding this comment.
The initial reason this went in is that I'm not super familiar with Python anymore, and the code here is written an a somewhat abstruse way. If I just look in isolation at, say, the version_key function, it's easy enough to see how it does the parsing, but more complicated to figure out how it translates that to the desired ordering. On the other hand it's very easy to look at test_versions_order_numerically_with_prereleases_first and note that it's doing the right thing.
And isn't the actual point of a test suite that changes to behavior will require a changes to all relevant tests, to prove they're intentional? Because otherwise, they fail on unintended changes in behavior.
My expectation is that neither the tool nor the test suite change very often. However, in the case that we do have to make changes, I think having the test suite will prove valuable.
Fixup commits should never be part of the initial PR submission. What is the value in keeping them separate as the review starts? |
They weren't, but then semgrep got antsy and I had to include at least one fixup so that I could point to the fixup commit as what fixed the thing it pointed out. And then I remembered that Python formatters/linters existed and thought to run them. |
This branch has not been deployed
2 participants
An alternative implementation of #2681.
Keeps the two-phase publishing process; delegates the heavy lifting to a Python script.
After merge, we need to prepare a test tag and see if this actually works. The expectation is that the publish will succeed, but it will be invisible to normal clients unless requested explicitly because it won't appear in the
maven-metadata.xml.