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

OCM-4460 | feat: Add actions for break glass credential #1855

Merged
merged 1 commit into from
Mar 26, 2024
Merged

OCM-4460 | feat: Add actions for break glass credential #1855

merged 1 commit into from
Mar 26, 2024

Conversation

chenz4027
Copy link
Contributor

@chenz4027 chenz4027 commented Mar 15, 2024 •
edited
Loading

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 15, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2024
@chenz4027 chenz4027 force-pushed the OCM-4460 branch 3 times, most recently from 28183f8 to a558a46 Compare March 19, 2024 19:35
@chenz4027 chenz4027 marked this pull request as ready for review March 19, 2024 19:36
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 42.74510% with 146 lines in your changes are missing coverage. Please review.

Project coverage is 21.79%. Comparing base (225cd4f) to head (c122bdc).
Report is 2 commits behind head on master.

Files Patch % Lines
pkg/breakglasscredential/flags.go 17.72% 63 Missing and 2 partials ⚠️
cmd/describe/breakglasscredential/cmd.go 46.05% 39 Missing and 2 partials ⚠️
cmd/create/breakglasscredential/cmd.go 32.60% 31 Missing ⚠️
pkg/output/output.go 0.00% 6 Missing ⚠️
pkg/ocm/break_glass_credential.go 93.75% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1855      +/-   ##
==========================================
+ Coverage   21.49%   21.79%   +0.30%     
==========================================
  Files         113      117       +4     
  Lines       18667    18963     +296     
==========================================
+ Hits         4012     4133     +121     
- Misses      14349    14519     +170     
- Partials      306      311       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andreadecorte
andreadecorte approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@andreadecorte andreadecorte left a comment

Choose a reason for hiding this comment

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

lgtm

robpblake
robpblake reviewed Mar 21, 2024
View reviewed changes
cmd/create/breakglasscredential/cmd.go Outdated Show resolved Hide resolved
pkg/breakglasscredential/flags.go Outdated Show resolved Hide resolved
pkg/breakglasscredential/flags.go Outdated Show resolved Hide resolved
pkg/breakglasscredential/flags.go Outdated Show resolved Hide resolved
pkg/breakglasscredential/flags.go Outdated Show resolved Hide resolved
cmd/describe/breakglasscredential/cmd.go Outdated Show resolved Hide resolved
cmd/describe/breakglasscredential/cmd.go Outdated Show resolved Hide resolved
cmd/list/breakglasscredential/cmd.go Outdated Show resolved Hide resolved
cmd/revoke/breakglasscredential/cmd.go Outdated Show resolved Hide resolved
pkg/breakglasscredential/flags.go Outdated Show resolved Hide resolved
@chenz4027 chenz4027 force-pushed the OCM-4460 branch 2 times, most recently from a1f20e6 to 672a632 Compare March 21, 2024 17:43
robpblake
robpblake requested changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@robpblake robpblake left a comment

Choose a reason for hiding this comment

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

As per discussion, slight rework to this PR to make the core workflows easier to handle.

@chenz4027
Copy link
Contributor Author

/retest

robpblake
robpblake reviewed Mar 25, 2024
View reviewed changes
cmd/create/breakglasscredential/cmd.go Outdated

ocm.AddClusterFlag(Cmd)

interactive.AddFlag(flags)
Copy link
Contributor

Choose a reason for hiding this comment

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

@chenz4027 You could just pass Cmd.Flags() here rather than assigning the flags variable. Not a biggie.

@chenz4027
Copy link
Contributor Author

Thanks @robpblake , I'll add more test coverage as suggested.

@chenz4027 chenz4027 force-pushed the OCM-4460 branch 5 times, most recently from 4975e84 to ea592c4 Compare March 25, 2024 20:20
@chenz4027
OCM-4460 | feat: Add actions for break glass credential
c122bdc 
Signed-off-by: Maggie Chen <magchen@redhat.com>

add the remaining functions

Signed-off-by: Maggie Chen <magchen@redhat.com>

remove delete to revoke

Signed-off-by: Maggie Chen <magchen@redhat.com>

add tests

Signed-off-by: Maggie Chen <magchen@redhat.com>

fix lint

Signed-off-by: Maggie Chen <magchen@redhat.com>

fix the json

Signed-off-by: Maggie Chen <magchen@redhat.com>

refactor

Signed-off-by: Maggie Chen <magchen@redhat.com>

refactor

Signed-off-by: Maggie Chen <magchen@redhat.com>

change to clusterkey

Signed-off-by: Maggie Chen <magchen@redhat.com>

refactor

Signed-off-by: Maggie Chen <magchen@redhat.com>

add poll

Signed-off-by: Maggie Chen <magchen@redhat.com>

add more test coverage

Signed-off-by: Maggie Chen <magchen@redhat.com>

fix fmt

Signed-off-by: Maggie Chen <magchen@redhat.com>

add test

Signed-off-by: Maggie Chen <magchen@redhat.com>

fix fmt

Signed-off-by: Maggie Chen <magchen@redhat.com>

add test

Signed-off-by: Maggie Chen <magchen@redhat.com>

use .id instead

Signed-off-by: Maggie Chen <magchen@redhat.com>
robpblake
robpblake approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@robpblake robpblake 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 Mar 26, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreadecorte, chenz4027, robpblake

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 Mar 26, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 26, 2024

@chenz4027: all tests passed!

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 92a2ed3 into openshift:master Mar 26, 2024
9 checks passed
@chenz4027 chenz4027 deleted the OCM-4460 branch May 9, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreadecorte andreadecorte andreadecorte approved these changes

@robpblake robpblake robpblake approved these changes

@gdbranco gdbranco Awaiting requested review from gdbranco

@willkutler willkutler Awaiting requested review from willkutler

Assignees

@robpblake robpblake

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chenz4027 @andreadecorte @robpblake