Skip to content
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

CLOUDP-229283: disable ownership detection #1371

Merged
merged 3 commits into from
Feb 14, 2024
Merged

CLOUDP-229283: disable ownership detection #1371

merged 3 commits into from
Feb 14, 2024

Conversation

s-urbaniak
Copy link
Collaborator

All Submissions:

  • Have you signed our CLA?
  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if there is one).
  • Update docs/release-notes/release-notes.md if your changes should be included in the release notes for the next release.

cmd/manager/main.go Outdated Show resolved Hide resolved
@s-urbaniak s-urbaniak force-pushed the CLOUDP-229283 branch 2 times, most recently from 7928035 to c6cb20d Compare February 12, 2024 20:04
Copy link
Contributor

github-actions bot commented Feb 12, 2024

Copy link
Collaborator

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

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

Great fix! 👍

Copy link
Collaborator

@josvazg josvazg left a comment

Choose a reason for hiding this comment

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

LGTM!

Be aware though, that this is missing the subobject checks. Those are not called IsOwner, but instead can...Reconcile, eg. canAuditingReconcile.

BTW, the IsOwner is used on top level objects that we are pretty sure will remove the check for good, with no replacement. Where as the can...Reconcile will need to be revisited with a proper alternative.

An useful regexp to find the affected code is can.*Reconcile which renders me 36 results in 12 files. I would vote to do that in a separate PR.

These are

@s-urbaniak
Copy link
Collaborator Author

@josvazg if there is something missing, we should definitely add it to this PR:

Be aware though, that this is missing the subobject checks. Those are not called isOwner, but instead can...Reconcile, eg. canUditingReconcile.

This definitely should be covered by setting SubObjectDeletionProtection: false at the reconciler struct level. Please advise what else we should do here.

@josvazg
Copy link
Collaborator

josvazg commented Feb 13, 2024

@josvazg if there is something missing, we should definitely add it to this PR:

Be aware though, that this is missing the subobject checks. Those are not called isOwner, but instead can...Reconcile, eg. canUditingReconcile.

This definitely should be covered by setting SubObjectDeletionProtection: false at the reconciler struct level. Please advise what else we should do here.

Yes, they are also disabled already, but maybe it would be good to mark the code as disabled explicitly?
We could skip and just go for a later removal. Not a big deal.

@josvazg
Copy link
Collaborator

josvazg commented Feb 13, 2024

We could skip and just go for a later removal. Not a big deal.

Lets do that and move on.

@s-urbaniak s-urbaniak merged commit 145b35e into main Feb 14, 2024
44 checks passed
@s-urbaniak s-urbaniak deleted the CLOUDP-229283 branch February 14, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants