Skip to content

Conversation

@flyrain
Copy link
Contributor

@flyrain flyrain commented May 10, 2025

It isn't used anywhere in the project, plus it causes potential extra maintenance cost like #1562.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be used by the NoSQL work.

@flyrain
Copy link
Contributor Author

flyrain commented May 12, 2025

I don’t think it makes sense to create a standalone module just for a potential future persistence implementation. Even if this class is needed for NoSQL support, it should live within the NoSQL module itself, not in a separate module that adds unnecessary structure and overhead. Let’s keep the module boundaries meaningful and aligned with actual usage.

@eric-maynard
Copy link
Contributor

Will it only be used by the NoSQL work? If not, maybe it can live outside the nosql module, but if so it should probably be contained there.

@snazy
Copy link
Member

snazy commented May 19, 2025

Use it for Minio, Keycloak, Postgres, etc etc etc.

@flyrain
Copy link
Contributor Author

flyrain commented May 19, 2025

Use it for Minio, Keycloak, Postgres, etc etc etc.

Just to clarify, Postgres doesn't use it, and Minio and Keycloak are not part of the Polaris repo.
We also have no clear timeline or confirmation on whether they will be added, how they'd be integrated, or if they even belong in the repo at all.

@snazy
Copy link
Member

snazy commented May 20, 2025

TBH, I don't understand why you want to remove something that's known to be used by work that supposed to go in. We do add "dangling" pieces for upcoming work all the time. I don't understand why you're pushing to remove exactly this part.

@flyrain
Copy link
Contributor Author

flyrain commented May 24, 2025

This isn’t just another piece of harmless scaffolding. It introduces public API.
Any public class becomes part of our compatibility contract. Once it lands, removing or reshaping it is a breaking change. That means we should only ship it when we’re confident in the design and need.

Dead code isn’t free.

  • Security scans still review it.
  • Binary and license audits include it.
  • New contributors waste time tracing unused paths.
We carry that cost every release until the real feature arrives, and “temporary” code has a habit of sticking around.

@eric-maynard
Copy link
Contributor

We do add "dangling" pieces for upcoming work all the time

I don't think I've personally ever added a dangling module for upcoming work, and I've scarcely seen it done in a way that wasn't super-specific to the upcoming work (e.g. a JDBC module for the upcoming JDBC work). We should try to minimize this, if possible.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jun 29, 2025
@dimas-b
Copy link
Contributor

dimas-b commented Jun 30, 2025

ContainerSpecHelper is used as of #1918

@dimas-b dimas-b closed this Jun 30, 2025
@github-project-automation github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants