Skip to content

ClusterResources should take the operator and controller names separately #482

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

Closed
4 tasks
nightkr opened this issue Oct 5, 2022 · 1 comment
Closed
4 tasks
Assignees
Labels

Comments

@nightkr
Copy link
Member

nightkr commented Oct 5, 2022

Currently, ClusterResources takes a single manager argument that must be unique per controller. The documentation suggests identifying both the controller and operator names (foo-operator_foo-controller), but in practice many single-controller operators just use the operator name (foo-operator).

This is not only inconsistent across the platform, but can lead to orphaned resources during upgrades that add more controllers (necessitating a switch to the expanded format). Additionally, it's generally a convention that un-namedspaced identifiers are reserved for the cluster administrators and users, third-party software should use the ns/id format for string keys (such as field managers or labels).

Acceptance

  • ClusterResources::new takes separate arguments for the operator and controller names
  • labels::get_recommended_labels takes separate arguments for the operator and controller names
  • managed-by labels and field managers follow the operator/controller format
  • The documentation is amended to encourage FQDN-style operator names (for example: zookeeper.stackable.tech rather than zookeeper-operator)
@nightkr nightkr moved this to Idea/Proposal in Stackable Engineering Oct 5, 2022
@lfrancke lfrancke moved this from Idea/Proposal to Refinement: Waiting for in Stackable Engineering Oct 14, 2022
@nightkr nightkr moved this from Refinement: Waiting for to Refinement: In Progress in Stackable Engineering Oct 17, 2022
@nightkr nightkr self-assigned this Oct 17, 2022
@nightkr nightkr moved this from Refinement: In Progress to Refinement Acceptance: Waiting for in Stackable Engineering Oct 17, 2022
@lfrancke lfrancke moved this from Refinement Acceptance: Waiting for to Ready for Development in Stackable Engineering Oct 17, 2022
@nightkr nightkr moved this from Ready for Development to Development: In Progress in Stackable Engineering Oct 17, 2022
nightkr added a commit that referenced this issue Oct 17, 2022
@nightkr nightkr moved this from Development: In Progress to Development: Waiting for Review in Stackable Engineering Oct 17, 2022
@siegfriedweber siegfriedweber moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Oct 19, 2022
@siegfriedweber siegfriedweber moved this from Development: In Review to Development: Waiting for Review in Stackable Engineering Oct 19, 2022
@sbernauer
Copy link
Member

Comment on the PR:

I would combine both tickets into one PR because they create a breaking change at the same place. If we would merge this PR and not immediately the PR for #491 then we would force people to adapt their code twice. But I would also not implement #491 hastily without a thourough refinement.

@soenkeliebau soenkeliebau moved this from Development: Waiting for Review to Development: Track in Stackable Engineering Oct 31, 2022
@nightkr nightkr moved this from Development: Track to Development: Waiting for Review in Stackable Engineering Nov 11, 2022
@bors bors bot closed this as completed in 2ce7c89 Nov 11, 2022
@sbernauer sbernauer moved this from Development: Waiting for Review to Acceptance: Waiting for in Stackable Engineering Nov 11, 2022
@lfrancke lfrancke moved this from Acceptance: Waiting for to Done in Stackable Engineering Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants