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

OSD-22639: prompt, store and reuse elevate reason #406

Merged
merged 3 commits into from
May 6, 2024

Conversation

Tof1973
Copy link
Contributor

@Tof1973 Tof1973 commented Apr 29, 2024

What type of PR is this?

feature/documentation/unit-test

What this PR does / Why we need it?

OSD-22639

When running ocm-backplate elevate with an empty string as reason, a prompt will be displayed (if possible) to request the reason.
When a reason is provided (as first argument, on prompt or from previous context), it will be stored in cluster context in order that if a reason is needed for elevation within the following 20 min for the same cluster, no prompt will be displayed and the stored context will be used.

Which Jira/Github issue(s) does this PR fix?

Resolves OSD-22639

Special notes for your reviewer

After this change, we will be able to update SOPs using elevate to specify an empty reason, and command will prompt for the reason.

Pre-checks (if applicable)

  • Ran unit tests locally
  • Validated the changes in a cluster
  • Included documentation changes with PR

@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 29, 2024

@Tof1973: This pull request references OSD-22639 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

What type of PR is this?

feature/documentation/unit-test

What this PR does / Why we need it?

OSD-22639

When running ocm-backplate elevate with an empty string as reason, a prompt will be displayed (if possible) to request the reason.
When a reason is provided (as first argument, on prompt or from previous context), it will be stored in cluster context in order that if a reason is needed for elevation within the following 20 min for the same cluster, no prompt will be displayed and the stored context will be used.

Which Jira/Github issue(s) does this PR fix?

Resolves OSD-22639

Special notes for your reviewer

After this change, we will be able to update SOPs using elevate to specify an empty reason, and command will prompt for the reason.

Pre-checks (if applicable)

  • Ran unit tests locally
  • Validated the changes in a cluster
  • Included documentation changes with PR

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 29, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2024

Codecov Report

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

Project coverage is 44.57%. Comparing base (51236e1) to head (9b8b250).
Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
- Coverage   44.74%   44.57%   -0.18%     
==========================================
  Files          62       63       +1     
  Lines        5113     5243     +130     
==========================================
+ Hits         2288     2337      +49     
- Misses       2510     2585      +75     
- Partials      315      321       +6     
Files Coverage Δ
cmd/ocm-backplane/elevate/elevate.go 66.66% <80.00%> (+66.66%) ⬆️
pkg/elevate/elevate.go 81.96% <66.66%> (-9.70%) ⬇️
pkg/elevate/elevate_context.go 73.80% <73.80%> (ø)
pkg/utils/util.go 45.80% <22.22%> (-3.76%) ⬇️

... and 1 file with indirect coverage changes

@Tof1973 Tof1973 force-pushed the store-reason branch 4 times, most recently from 9433e7b to 28cbc22 Compare April 29, 2024 13:15
@Tof1973
Copy link
Contributor Author

Tof1973 commented Apr 30, 2024

OSD-22250

@iamkirkbater
Copy link
Contributor

I'll give my +1 to this here - however it will be up to the backplane CLI maintainers to review further and merge.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2024
@samanthajayasinghe
Copy link
Contributor

This is not a blocker, but what if include Elevate section in the readme and document what elevate command does, especially X mins timeouts

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 3, 2024
@Tof1973
Copy link
Contributor Author

Tof1973 commented May 3, 2024

This is not a blocker, but what if include Elevate section in the readme and document what elevate command does, especially X mins timeouts

I initially have not updated README, as there were no elevate section, but here I created a new dedicated elevate section with all details...

Copy link
Contributor

@hectorakemp hectorakemp left a comment

Choose a reason for hiding this comment

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

Great work @Tof1973!
All of this makes sense to me - but there's just a few nits in readme related to typos that are simple fixes. I also have tried testing it and can't quite get it to work the way it is expected to in the readme - see this slack thread: https://redhat-internal.slack.com/archives/C016S65RNG5/p1714745369106709?thread_ts=1714393862.376999&cid=C016S65RNG5

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Tof1973 Tof1973 force-pushed the store-reason branch 2 times, most recently from ca7554a to bb3749b Compare May 3, 2024 16:05
Copy link
Contributor

openshift-ci bot commented May 3, 2024

@Tof1973: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test build
  • /test coverage
  • /test images
  • /test lint
  • /test test

The following commands are available to trigger optional jobs:

  • /test scan-optional

Use /test all to run all jobs.

In response to this:

/retest all

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.

Copy link
Contributor

openshift-ci bot commented May 6, 2024

@Tof1973: 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.

@hectorakemp
Copy link
Contributor

/approve

@hectorakemp
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2024
Copy link
Contributor

openshift-ci bot commented May 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorakemp, iamkirkbater, Tof1973

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 May 6, 2024
Copy link
Contributor

openshift-ci bot commented May 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorakemp, iamkirkbater, Tof1973

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-merge-bot openshift-merge-bot bot merged commit 812bc2f into openshift:main May 6, 2024
7 checks passed
@Tof1973 Tof1973 deleted the store-reason branch May 6, 2024 11:45
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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants