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

✨ Add command for fetching permission claims #2203

Merged

Conversation

varshaprasad96
Copy link
Contributor

@varshaprasad96 varshaprasad96 commented Oct 17, 2022

Summary

This PR adds command to get permission claims related to an APIBinding.
Example:

➜  kcp git:(cmd/permission-claim-get) ✗ kubectl-kcp claims get apibinding cert-manager
NAME                       BINDING            STATUS
group1-resource1    cert-manager   Rejected
group2-resource2   cert-manager   Rejected
➜  kcp git:(cmd/permission-claim-get) ✗ kubectl-kcp claims get apibinding
NAME                       BINDING            STATUS
group1-resource1    cert-manager   Rejected
group2-resource2   cert-manager   Rejected
group3-resource3   solo                   Accepted

Related design doc: https://docs.google.com/document/d/1J31wXY1-2aCyyGFjUlusKoHDzV-zktAmRdIMjMf4OR0/edit

@varshaprasad96 varshaprasad96 changed the title ✨ Add commands for permission claims ✨ Add command for fetching permission claims Oct 17, 2022
Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

Do we have some example output?

pkg/cliplugins/claims/cmd/cmd.go Show resolved Hide resolved
pkg/cliplugins/claims/plugin/claims.go Outdated Show resolved Hide resolved
out := printers.GetNewTabWriter(g.Out)
defer out.Flush()

err = printHeaders(out)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we support an -o json or --quiet? Can I pipe the output of this command into another command that accepts the claims?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently no. kubectl get accepts K8s objects itself as args. For example, having the output schema of pod or deployment would add value. We don't have an independent custom resource named permission claims.
Instead edit will reopen the entire APIBinding resource in an editor and allow users edit it. So there is no specific use case of piping the output to any other command directly and modifying the status of permission claim.

@varshaprasad96
Copy link
Contributor Author

@stevekuznetsov a super basic sample output is in the PR description (github is screwing up the formatting). It is basically the output of whatever I am specifying in apibinding.spec.permisisonclaims

NAME                        BINDING             STATUS
corev1-configmap    cert-manager    Rejected
corev1-secret           cert-manager     Rejected

@fgiloux
Copy link
Contributor

fgiloux commented Oct 20, 2022

@stevekuznetsov a super basic sample output is in the PR description (github is screwing up the formatting). It is basically the output of whatever I am specifying in apibinding.spec.permisisonclaims

NAME                        BINDING             STATUS
corev1-configmap    cert-manager    Rejected
corev1-secret           cert-manager     Rejected

Hi @varshaprasad96, how will it look like when name/namespace or selectors have been defined? Will they get added to the first column? What about verbs and subresources that may come later on? Will the verbs be in an additional column? It should not prevent this PR to get progressed but it may be interesting to have in mind what will come next.
What about naming the first column RESOURCES or CLAIMS if verbs were to be added to it later on? My feeling is that NAME does not carry much information.

@stevekuznetsov
Copy link
Contributor

Yep - would love to have the leading column be the APIBinding name, and then have a column for group/version, and another for resource(s)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2022
@varshaprasad96
Copy link
Contributor Author

@fgiloux @stevekuznetsov Have modified the first column to be the APIBINDING name and the second to be RESOURCE GROUP-VERSION. We haven't decided on what the UI would like when additional fields like verbs/subresources/namespaces are added. We could either have them here by adding another column in future, or probably have just the sub resources included here and verbs in a metadata/additional-details column.

Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

IIRC we're about to make resource:group not 1:1 in permission claims but when that happens they can fiddle with the columns here.

@stevekuznetsov
Copy link
Contributor

/lgtm
/approve

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

openshift-ci bot commented Oct 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2022
@openshift-merge-robot openshift-merge-robot merged commit bb0332e into kcp-dev:main Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants