-
Notifications
You must be signed in to change notification settings - Fork 23
Add APIBinding to make controller work #26
base: main
Are you sure you want to change the base?
Add APIBinding to make controller work #26
Conversation
Hi @vincent-pli. Thanks for your PR. I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@vincent-pli could you remove this if we're adding this to the config itself? Does |
/ok-to-test |
Ignore that e2e :| |
10c5f71
to
de63d08
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vincent-pli The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@stevekuznetsov I have remove that code snippet and did some tiny update, the |
de63d08
to
eac083b
Compare
@vincent-pli: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
exportName: data.my.domain | ||
acceptedPermissionClaims: | ||
permissionClaims: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion we should start tagging versions of the example with the release match.
go.mod is still with github.com/kcp-dev/kcp/pkg/apis v0.7.0, which has acceptedPermissionClaims
There is PR #30 that addresses the update of the versions. IMHO PR #30 could be merged first, which would accommodate the change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 to making sure the releases match up to KCP's
For what it's worth kcp-dev/kcp#2135 has merged. As it is only in the main branch for now I still think that there is value in this PR. |
PR needs rebase. 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. |
/uncc |
Resolve issue #25