Skip to content

Maybe get_recommended_labels should take a struct of named arguments #491

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
2 tasks
nightkr opened this issue Oct 17, 2022 · 7 comments
Closed
2 tasks

Comments

@nightkr
Copy link
Member

nightkr commented Oct 17, 2022

The current list of string arguments is confusing to follow (especially if you don't use rust-analyzer with parameter inlays), and is confusing and error-prone to update in operators when new arguments are added (often in the middle of the list).

This PR adds another parameter to the list: https://github.com/stackabletech/operator-rs/pull/492/files#diff-f3a8a77aa3434fb4aef8eb9c3d011832fc27301972b9975aa303832c3ed33675 Clippy also complains about the many parameters.

This is the function in its current state: https://github.com/stackabletech/operator-rs/blob/main/src/labels.rs#L22

Acceptance criteria:

  • get_recommended_labels does not take so many arguments
  • with_recommended_labels also
@nightkr nightkr added type/feature-improvement release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Oct 17, 2022
@nightkr nightkr moved this to Idea/Proposal in Stackable Engineering Oct 17, 2022
@lfrancke lfrancke moved this from Idea/Proposal to Refinement: Waiting for in Stackable Engineering Oct 26, 2022
@nightkr
Copy link
Member Author

nightkr commented Nov 1, 2022

I actually kind of think even just doing the easy "struct of &strs would be a huge improvement, but I believe @siegfriedweber had strong opinions here so I'll wait for him to return...

@lfrancke lfrancke moved this from Refinement: Waiting for to Next in Stackable Engineering Nov 7, 2022
@lfrancke lfrancke moved this from Next to Refinement: In Progress in Stackable Engineering Nov 7, 2022
@lfrancke lfrancke moved this from Refinement: In Progress to Next in Stackable Engineering Nov 7, 2022
@lfrancke lfrancke moved this from Next to Refinement: In Progress in Stackable Engineering Nov 7, 2022
@lfrancke lfrancke moved this from Refinement: In Progress to Next in Stackable Engineering Nov 7, 2022
@siegfriedweber
Copy link
Member

A struct would be an improvement because the field names help to assign the correct values and the struct can be reused so that repetition is reduced.

I had a more elaborate solution in mind where you just pass the cluster spec and a "controller spec" where the recommended labels are derived from. The ClusterResources already contain some of the required information. The application version could be made accessible via a Trait. The labels for the role and role-group could be extracted into another builder function because not all resources are at a role-group level and they currently use "global" as a placeholder. If the recommended labels do not match exactly what the caller needs then she or he can overwrite them with other functions in the ObjectMetaBuilder.

But after a closer look, the implementation of my solution is not as straight forward as I hoped. So I am also fine with the easy struct solution.

@fhennig fhennig moved this from Next to Refinement: In Progress in Stackable Engineering Nov 8, 2022
@nightkr
Copy link
Member Author

nightkr commented Nov 8, 2022

Is there much further refinement planned here then?

@fhennig
Copy link
Contributor

fhennig commented Nov 8, 2022

We could still use a trait instead of a struct and then in the trait we could implement functions like the superset_version and we could hardcode some things in there, instead of having the OPERATOR_NAME static vars etc.

But it's not a strong opinion

@fhennig
Copy link
Contributor

fhennig commented Nov 8, 2022

I just asked Sigi and he said a Trait might be nice, but he'd leave it up to the implementor.

@fhennig
Copy link
Contributor

fhennig commented Nov 8, 2022

I've added 2 acceptance criteria

@fhennig fhennig moved this from Refinement: In Progress to Refinement Acceptance: Waiting for in Stackable Engineering Nov 8, 2022
@lfrancke
Copy link
Member

lfrancke commented Nov 8, 2022

The description does not really talk about the solution but the details are in the comments so I'm fine with it.

@lfrancke lfrancke moved this from Refinement Acceptance: Waiting for to Ready for Development in Stackable Engineering Nov 8, 2022
@nightkr nightkr moved this from Ready for Development to Development: In Progress in Stackable Engineering Nov 10, 2022
@nightkr nightkr self-assigned this Nov 10, 2022
@nightkr nightkr moved this from Development: In Progress to Development: Waiting for Review in Stackable Engineering Nov 10, 2022
@sbernauer sbernauer self-assigned this Nov 11, 2022
@sbernauer sbernauer moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Nov 11, 2022
@bors bors bot closed this as completed in f334143 Nov 11, 2022
@sbernauer sbernauer moved this from Development: In 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
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants