Skip to content

[Merged by Bors] - Separate operator and controller names #492

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
wants to merge 10 commits into from

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Oct 17, 2022

Description

Fixes #482

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@nightkr nightkr added type/bug release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Oct 17, 2022
Comment on lines 156 to 159
operator_name: &str,
controller_name: &str,
app_role: &str,
app_role_group: &str,
Copy link
Member Author

@nightkr nightkr Oct 17, 2022

Choose a reason for hiding this comment

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

This triggers a cargo warning due to #491, not sure if we want to tackle that in this PR...

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with munging it together but I'd appreciate it if you could bring it up at the daily or arch meeting to see if there are any objections (I don't expect any tbh.).
If you decide to let me know please.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to account for #491 being merged.

@nightkr nightkr requested a review from a team October 17, 2022 09:35
@siegfriedweber siegfriedweber requested review from siegfriedweber and removed request for a team October 19, 2022 09:11
bors bot pushed a commit that referenced this pull request Nov 11, 2022
## Description

Hopefully this should help clarify how the different names hold together, and help make it clearer how to adjust for later changes going forward.

This doesn't cover `role_group_selector_labels` or `role_selector_labels` since they have a more obvious inherent structure to them, and I don't see them changing as much (especially since they are effectively part of the public API).

This is kind of coupled to #492, do we want to merge that into this PR or do that one on top of this afterwards?

Fixes #491.



Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@stackable.de>
@nightkr nightkr self-assigned this Nov 11, 2022
@nightkr nightkr requested a review from a team November 11, 2022 11:00
@nightkr
Copy link
Member Author

nightkr commented Nov 11, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 11, 2022

## Description

Fixes #482



Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@stackable.de>
@bors
Copy link
Contributor

bors bot commented Nov 11, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Separate operator and controller names [Merged by Bors] - Separate operator and controller names Nov 11, 2022
@bors bors bot closed this Nov 11, 2022
@bors bors bot deleted the fix/cr-separate-operator-and-controller-names branch November 11, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClusterResources should take the operator and controller names separately
4 participants