Skip to content

[Merged by Bors] - Orphaned resources handling #215

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 23 commits into from
Closed

[Merged by Bors] - Orphaned resources handling #215

wants to merge 23 commits into from

Conversation

siegfriedweber
Copy link
Member

@siegfriedweber siegfriedweber commented Jul 18, 2022 •
edited
Loading

Description

Delete orphaned resources

Closes #154

Integration tests

Build Status

Review Checklist

  • Code contains useful comments
  • CRD change approved (or not applicable)
  • (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)
  • Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information
@siegfriedweber siegfriedweber requested a review from a team July 18, 2022 14:55
@siegfriedweber siegfriedweber self-assigned this Jul 18, 2022
@siegfriedweber siegfriedweber mentioned this pull request Jul 18, 2022
Closed
5 tasks
@siegfriedweber siegfriedweber marked this pull request as ready for review August 4, 2022 08:38
@adwk67 adwk67 self-requested a review August 4, 2022 10:33
@adwk67 adwk67 assigned adwk67 and unassigned siegfriedweber Aug 4, 2022
@adwk67 adwk67 removed the request for review from a team August 4, 2022 10:34
sbernauer
sbernauer reviewed Aug 4, 2022
View reviewed changes
@sbernauer
Copy link
Member

sbernauer commented Aug 4, 2022 •
edited
Loading

Sorry @adwk67 for not marking the begin of the review correctly but it was quickly because i already know the PR from before releasing operator-rs
sbernauer
sbernauer approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Please wait with merging till https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/hbase-operator-it-custom/22/console suceeded
Approving now, as it's your last day and this is a reference-implementation
soenkeliebau
soenkeliebau approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@soenkeliebau soenkeliebau left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have not run the tests due to time restrictions, but Sigi and I spoke about this offline and I have created an equivalent PR for NiFi that looks very similar, so am happy to approve this as is :)
@sbernauer
Copy link
Member

Nice thing with the error files! Integration-tests passed so feel free to merge :)
@siegfriedweber
Copy link
Member Author

bors merge
bors bot pushed a commit that referenced this pull request Aug 4, 2022
@siegfriedweber
Orphaned resources handling (#215)
97f73e9 
# Description Delete orphaned resources
@bors
Copy link
Contributor

bors bot commented Aug 4, 2022

Pull request successfully merged into main.

Build succeeded:
@bors bors bot changed the title Orphaned resources handling [Merged by Bors] - Orphaned resources handling Aug 4, 2022
@bors bors bot closed this Aug 4, 2022
@bors bors bot deleted the orphaned_resources_handling branch August 4, 2022 17:22
@soenkeliebau soenkeliebau mentioned this pull request Aug 4, 2022
Closed
7 tasks
bors bot pushed a commit to stackabletech/nifi-operator that referenced this pull request Aug 11, 2022
@soenkeliebau
Orphaned resources handling (#319)
8e4b5ac 
# Description Ensure the operator correctly deletes orphaned resources. Closes #244 sister PR that was modelled close to stackabletech/hbase-operator#215
nightkr
nightkr reviewed Sep 29, 2022
View reviewed changes
rust/operator-binary/src/hbase_controller.rs
use strum::{EnumDiscriminants, IntoStaticStr};

const FIELD_MANAGER_SCOPE: &str = "hbasecluster";
const CONTROLLER_NAME: &str = "hbase-operator";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the same. The field manager needs to identify the controller within the operator.
Copy link
Member Author

Choose a reason for hiding this comment

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

"hbase-operator" is the controller name in this case. As there is only one controller in this operator, it should not pose a problem. But if you see a problem then please create a ticket. I agree that this naming is a bit confusing. So we could provide both, operator name and controller name, to ClusterResources.
Copy link
Contributor

Choose a reason for hiding this comment

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

stackabletech/operator-rs#482
@adwk67 adwk67 mentioned this pull request Oct 5, 2022
Closed
7 tasks
@nightkr nightkr mentioned this pull request Oct 5, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants

@siegfriedweber @sbernauer @nightkr @soenkeliebau @adwk67