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

Start deprecation of --record flag #102873

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Start deprecation of --record flag #102873

merged 1 commit into from
Jun 16, 2021

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Jun 15, 2021 •
edited
Loading

What type of PR is this?

/kind cleanup
/kind deprecation
/sig cli
/priority backlog

What this PR does / why we need it:

This is a follow up from a discussion at SIG-CLI some time ago and email send to k/dev. I haven't heard any complaints and with new mechanism which sends commands through HTTP headers entering beta and very rich auditing functionality available in kubernetes it's time to deprecate --record flag which wasn't used consistently across all commands and is rather confusing to a lot of users.

Special notes for your reviewer:

/assign @eddiezane

Does this PR introduce a user-facing change?

Deprecate --record flag in kubectl. The --record flag is being replaced with the mechanism from https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/859-kubectl-headers which annotates HTTP requests with kubectl command details.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. sig/cli Categorizes an issue or PR as relevant to SIG CLI. priority/backlog Higher priority than priority/awaiting-more-evidence. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 15, 2021
@k8s-ci-robot
Copy link
Contributor

@soltysh: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 15, 2021
@k8s-ci-robot k8s-ci-robot requested review from deads2k and seans3 June 15, 2021 08:21
@soltysh
Copy link
Contributor Author

soltysh commented Jun 15, 2021

/retest

sftim
sftim reviewed Jun 15, 2021
View reviewed changes
staging/src/k8s.io/cli-runtime/pkg/genericclioptions/record_flags.go
@@ -93,6 +93,7 @@ func (f *RecordFlags) AddFlags(cmd *cobra.Command) {

if f.Record != nil {
cmd.Flags().BoolVar(f.Record, "record", *f.Record, "Record current kubectl command in the resource annotation. If set to false, do not record the command. If set to true, record the command. If not set, default to updating the existing annotation value only if one already exists.")
cmd.Flags().MarkDeprecated("record", "--record will be removed in the future")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cmd.Flags().MarkDeprecated("record", "--record will be removed in the future")
cmd.Flags().MarkDeprecated("record", "--record will be removed in a future Kubernetes release")

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's consistent with all the rest of deprecation messages so I left is as is.

@sftim
Copy link
Contributor

sftim commented Jun 15, 2021

Changelog nit; can I suggest:

Deprecate --record flag in kubectl. The --record flag is being replaced with the mechanism from https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/859-kubectl-headers which annotates HTTP requests with kubectl command details.

@soltysh
Copy link
Contributor Author

soltysh commented Jun 15, 2021

Changelog nit; can I suggest:

Good point - updated.

@soltysh
Copy link
Contributor Author

soltysh commented Jun 15, 2021

/retest

@eddiezane
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eddiezane, soltysh

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

@eddiezane
Copy link
Member

Flakes might be #100331 #97071.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit a72296f into kubernetes:master Jun 16, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 16, 2021
@BenTheElder BenTheElder mentioned this pull request Jun 23, 2021
Open
@BenTheElder
Copy link
Member

xref: #40422

@soltysh soltysh deleted the deprecate_record branch July 6, 2021 13:32
@Firehed Firehed mentioned this pull request Aug 12, 2021
Open
@pacoxu pacoxu mentioned this pull request Sep 16, 2021
Closed
@eddiezane eddiezane mentioned this pull request Oct 6, 2021
Merged
@abyssmemes
Copy link

Did you have alternative?

@BenTheElder
Copy link
Member

see the section "What this PR does / why we need it:" in the PR description #102873 (comment)

TLDR: audit logging + kubernetes/enhancements#859

@tanvp112
Copy link

tanvp112 commented Mar 6, 2022 •
edited
Loading

@BenTheElder how does the audit logging apply here, hopefully this doesn't mean we need to browse thru the audit log for history command?

The other link sounds more reasonable, but any idea how to use it with kubectl rollout history and undo already?

Hopefully --record will not be remove until it is clear what exactly is the alternative and how to use them. If not this is actually more confuse than --record flag itself.

@squaricdot
Copy link

Would like to mention that using the record function is also still part of the linuxfoundation CKA curriculum at the moment.

@vjunior1981
Copy link

@olwenya just tested like this and it worked:

~$ kubectl create deploy nginx --image=nginx --replicas=2
deployment.apps/nginx created
~$ kubectl set image deploy/nginx nginx=nginx:1.19
deployment.apps/nginx image updated
~$ kubectl annotate deploy/nginx kubernetes.io/change-cause='update image to 1.19'
deployment.apps/nginx annotated
~$ kubectl rollout history deploy/nginx
deployment.apps/nginx 
REVISION  CHANGE-CAUSE
1         <none>
2         update image to 1.19

Not perfect, but is an alternative.

@wangscot092019 wangscot092019 mentioned this pull request May 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@eddiezane eddiezane eddiezane approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@seans3 seans3 Awaiting requested review from seans3

Assignees

@eddiezane eddiezane

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.
10 participants
@soltysh @k8s-ci-robot @sftim @eddiezane @fejta-bot @BenTheElder @abyssmemes @tanvp112 @squaricdot @vjunior1981