-
Notifications
You must be signed in to change notification settings - Fork 4
Server retired packages #356
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a "retired packages" feature that allows server administrators to mark specific packages as retired. Retired packages are filtered out from most API endpoints by default but can still be accessed when explicitly requested.
- Added CLI options
--retiredPackageand--retiredFileto specify retired packages for the server - Modified API endpoints to filter out retired packages unless
includeRetiredparameter is provided - Retired packages remain accessible via direct ZIP download and individual package pages
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Poseidon/CLI/Serve.hs | Implements core retired package filtering logic and updates API endpoints |
| src/Poseidon/CLI/OptparseApplicativeParsers.hs | Adds CLI parsers for retired package options |
| src-executables/Main-trident.hs | Integrates retired package parser into serve command |
| src/Poseidon/EntityTypes.hs | Adds Show instance for EntityInput type |
| test/PoseidonGoldenTests/GoldenTestsRunCommands.hs | Updates test calls to include empty retired packages list |
| poseidon-hs.cabal | Bumps version to 1.6.8.0 |
| CHANGELOG.md | Documents the new feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
One point to note: I reused the |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
OK I've now tested it and tweaked some more things, for example the explorer page now shows also the correct number of packages if retired ones are excluded. You can always add |
|
Thanks for addressing this, finally! This is a very important feature for the entire ecosystem! Before I try it out and look at the code I have a first big question: Shouldn't we turn the CLI options poseidon-hs/src/Poseidon/CLI/Serve.hs Lines 88 to 95 in 987b8d7
Filtering out retired packages is something that should imho happen on the archive level, not above. Ultimately my idea is as follows: Every archive should have a separate list of retired packages, tracked with Git in the respective archive repository (then it's perfectly transparent what was retired when and by whom). If |
|
Excellent idea!!! I will get on with that suggestion! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #356 +/- ##
==========================================
+ Coverage 56.83% 56.96% +0.12%
==========================================
Files 33 33
Lines 4993 5052 +59
Branches 546 550 +4
==========================================
+ Hits 2838 2878 +40
- Misses 1609 1624 +15
- Partials 546 550 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
OK, I've made this change now. I've reformatted the |
|
Sorry for the reformatting of Serve.hs, I realize now that this is a bit hard to review. |
nevrome
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me 👍 - great! Didn't test it yet, though.
nevrome
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized now that the mechanism you implemented is to read retiredPackages directly from the archive config .yml file. I fear that does not cover our specific usecase. We need a way to store retired packages IN the archive Git repositories to document transparently which packages were retired when, by whom (and potentially why).
In my opinion the .yml file should allow for optional file paths to per-archive retiredPackagesFiles. Then we can easily maintain such a file in the archives. The format for this file could also be a .yml file for consistency, and to reuse the parsing code you have already written. Bonus points for a field in said file that allows to quickly document WHY a given package was retired.
I'm also OK with other solutions, but I'm currently convinced that the retired package list should live in the archives.
|
Beyond that things worked as expected in my test 👍. Nice! |
|
OK, so I've fixed all the other small points you've raised (all much appreciated!). Your last point I also agree with. But for simplicity sake I would then suggest to only add a retiredFile option in the YAML, and not both a direct listing and a file. So I will just replace the current option "retiredPackages" by a new option "retiredPackagesFile" which would then be a YAML list. YAML has the advantage that we can add comments, over straight text files. |
|
Sounds good 👍. As soon as this is implemented we can test it on the test server with the community-archive. The old How would you like to structure the title: Poseidon community-archive retired package list
version: 1.58.0
lastModified: 2025-10-15
packages:
- title: 2019_BraceNatureEcologyEvolution
version: X.X.X
comment: Package was merged with 2019_Brace_Britain.
- ...Not sure if we need a version number here. The chronicle files in the archives are currently all called |
|
Yes, I think a YAML format makes sense. That saves me a bit trouble in parsing entities, like I had originally, because then we can just list the version and the package title right in the YAML. I like it. And |
|
OK, this is finally done. Sorry that it took me so long. |
|
Excellent! I went for a field test right away this time. The test server at https://testserver.poseidon-adna.org is now running on this trident version. I also added an When I check here https://testserver.poseidon-adna.org/explorer/community-archive the package is indeed not displayed any more. I can still access it with the URL: https://testserver.poseidon-adna.org/explorer/community-archive/2019_BraceNatureEcologyEvolution/latest. That looks all fine. On the command line I get the following expected results: Should there be an option for |
Addresses #325. Here is the Changelog entry: