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.

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

🐛 Fix permissionclaim patch thrashing #2174

Merged
merged 1 commit into from
Oct 11, 2022
Merged

🐛 Fix permissionclaim patch thrashing #2174

merged 1 commit into from
Oct 11, 2022

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Oct 11, 2022

Summary

  • Add map keys to APIBinding.Status.AppliedPermissionClaims and .ExportPermissionClaims
  • Switched to using a sorted list when calculating applied permission claims so it doesn't keep generating new patches because of different ordering in the list.

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 11, 2022
@ncdc
Copy link
Member Author

ncdc commented Oct 11, 2022

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2022
@ncdc
Copy link
Member Author

ncdc commented Oct 11, 2022

/cherry-pick release-0.9

@openshift-cherrypick-robot
Copy link

@ncdc: once the present PR merges, I will cherry-pick it on top of release-0.9 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

csams
csams approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@csams csams left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csams, ncdc, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ncdc
Fix permissionclaim patch thrashing
f0bb055 
- Add map keys to APIBinding.Status.AppliedPermissionClaims and
  .ExportPermissionClaims
- Switched to using a sorted list when calculating applied permission
  claims so it doesn't keep generating new patches because of different
  ordering in the list.

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
@ncdc ncdc force-pushed the fix-permissionclaim-patch-thrashing branch from 6921d77 to f0bb055 Compare October 11, 2022 20:53
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2022
@stevekuznetsov
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2022
@openshift-merge-robot openshift-merge-robot merged commit a8cd9b1 into kcp-dev:main Oct 11, 2022
@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Oct 11, 2022
Merged
@openshift-cherrypick-robot
Copy link

@ncdc: new pull request created: #2175

In response to this:

/cherry-pick release-0.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sttts
Copy link
Member

sttts commented Oct 12, 2022

This PR is wrong. It might be a hot-fix, but it's the wrong direction.

@sttts
Copy link
Member

sttts commented Oct 12, 2022

/revert

@sttts sttts mentioned this pull request Oct 12, 2022
Closed
sttts
sttts reviewed Oct 12, 2022
View reviewed changes
pkg/apis/apis/v1alpha1/types_apibinding.go
AppliedPermissionClaims []PermissionClaim `json:"appliedPermissionClaims,omitempty"`

// exportPermissionClaims records the permissions that the export provider is asking for
// the binding to grant.
// +optional
// +listType=map
Copy link
Member

Choose a reason for hiding this comment

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

this must be atomic

Copy link
Member

Choose a reason for hiding this comment

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

if atomic broke something, then the patch code is wrong.

sttts
sttts reviewed Oct 12, 2022
View reviewed changes
pkg/apis/apis/v1alpha1/types_apibinding.go
@@ -174,11 +174,17 @@ type APIBindingStatus struct {
// state in spec.permissionClaims.
//
// +optional
// +listType=map
Copy link
Member

Choose a reason for hiding this comment

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

this must be atomic

Copy link
Member

Choose a reason for hiding this comment

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

the spec one can be set.

@s-urbaniak s-urbaniak mentioned this pull request Oct 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@csams csams csams approved these changes

@stevekuznetsov stevekuznetsov stevekuznetsov approved these changes

@s-urbaniak s-urbaniak Awaiting requested review from s-urbaniak

@shawn-hurley shawn-hurley Awaiting requested review from shawn-hurley

Assignees

@csams csams

@stevekuznetsov stevekuznetsov

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ncdc @openshift-cherrypick-robot @stevekuznetsov @sttts @csams @openshift-merge-robot