Proposal for porcelain commands to make working with the last-applied… #287

pwittrock · 2017-01-25T01:53:51Z

…-config easier

pwittrock · 2017-01-25T01:57:02Z

cc @kubernetes/sig-cli-api-reviews

@liggitt @fabianofranz @adohe WDYT of this?

mengqiy · 2017-01-26T00:50:00Z

kubectl edit will update the annotation if the last-applied-config already exists. It will save almost everything in the last-applied-config. It will cause trouble for apply to manage the fields.
So we may need to change edit's behavior.

liggitt · 2017-01-26T00:51:27Z

So we may need to change edit's behavior.

why? if a user edits a resource, is it not essentially an in-place apply?

mengqiy · 2017-01-26T01:09:17Z

IMO one of the ideas in this proposal is that last-applied-config should only keep the fields that is manged by apply. e.g. if we want an autoscaler to manage a field like replica count, we should not save it in the annotation.

edit is doing a 2-way diff, and it will save almost all fields in the annotation. This behavior may cause trouble to the idea above.

pwittrock · 2017-01-26T01:09:56Z

Are you familiar with use cases for running edit against an object managed by apply? Admittedly, I don't know much about the use cases for this.

why? if a user edits a resource, is it not essentially an in-place apply?

How do you define apply in this context? As I understand it, apply is a utility for managing a kubernetes object using a local configuration file. In the case of edit, no configuration file is used.

Since the reason for the annotation is to identify fields deleted from a local file - so that a patch can be sent to clear them from the live object. In the case of edit, the local configuration is not updated unless it is done so out of band. Why would a user do this instead of updating the configuration file?

pwittrock · 2017-01-26T17:03:45Z

edit is doing a 2-way diff, and it will save almost all fields in the annotation. This behavior may cause trouble to the idea above.

Good catch. This is definitely wrong. We don't want it to do this.

thockin · 2017-01-27T06:04:13Z

We have a lot of addons that are apply'ed into existence. End-users seem to find new ways to hurt themselves on every release by changing the value of some field that we didn't specify. Often args or env or things like that, it seems. They get "unhappy" when we nuke those changes.

pwittrock · 2017-01-27T17:49:47Z

@thockin Is your comment specifically about edit, or does it relate to the other parts of this proposal? I agree we should fix edit. @ymqytw Would you file an issue about this and take it as an item for 1.6. Also please follow up with @liggitt to make sure his concerns are satisfied. Add it as an item to the sig-cli agenda for next week.

@thockin FWIW, I am starting to write another proposal for how to address the issue around editing fields that were specified in the configuration used by apply. Essentially: Any kubectl command besides apply that writes to an object checks for the last-applied-config annotation and provides a warning before updating fields present in the annotation.

pwittrock · 2017-01-27T17:52:04Z

cc @kubernetes/sig-cli-misc @kubernetes/sig-cli-feature-requests

Any one else still want to review this? I'd like to be able to get actionable feedback or approval by the sig-cli meeting next Wednesday.

liggitt · 2017-01-27T19:42:05Z

Any kubectl command besides apply that writes to an object checks for the last-applied-config annotation and provides a warning before updating fields present in the annotation.

so if I use apply, I can't use label, annotate, edit, etc? that's unfortunate

mengqiy · 2017-01-27T19:50:34Z

so if I use apply, I can't use label, annotate, edit, etc? that's unfortunate

IMO if label and annotate are updating a field that is not owned by apply (not in the last-applied-config annotation), that operation is OK.
@pwittrock has a related doc PR to teach the users how to manage the resource.

mengqiy · 2017-01-27T21:15:17Z

@pwittrock created kubernetes/kubernetes#40626

pwittrock · 2017-01-27T21:44:33Z

@liggitt

so if I use apply, I can't use label, annotate, edit, etc? that's unfortunate

That is not the intention, but the current implementation of edit breaks kubectl apply. We need to follow up on annotate, label, scale, set, etc and get everything working consistently.

The reason edit is broken now is that the last-applied-config is essentially a list of fields to delete if they do not appear in the configuration file provided to apply. Having edit update the annotation essentially says "have apply delete all my changes if I don't add them back to the configuration file before apply is next called". This seems like the wrong behavior to me, and IMHO the proper behavior is to have apply ignore any changes made by edit so long as they don't conflict with the fields set in the configuration file.

Let me know if it would be helpful to go through a few use cases and the resulting state.

pwittrock · 2017-01-27T22:12:58Z

@liggitt FYI, I have this proposal for better support around using apply with imperative kubectl commands (edit, set, label, scale, annotate, patch, etc) #303

pwittrock · 2017-01-30T21:51:53Z

@kubernetes/sig-cli-proposals I've added this as an agenda item to the meeting on Wednesday and would like to merge it afterward if there are no remaining open comments.

liggitt · 2017-01-30T22:18:21Z

contributors/design-proposals/sig-cli/kubectl_apply_getsetdiff_last_applied_config.md

+1. Apply subcommands
+  - `kubectl apply set/view/diff last-applied
+  - a little bit odd to have 2 verbs in a row
+  - improves discoverability to have these as subcommands so they are tied to apply


how would subcommands interact with resource-builder parsing of args? ran into this with an attempt at a configmap-specific edit subcommand

Great question. How did you resolve it?

This should only accept files, and not accept type/name for resources. Is it possible to configure the resource-builder in this manner? I assume so since it seems to work for apply.

ah, looks like only the builders that specify ResourceTypeOrNameArgs(true, args...) consume arbitrary args (like get, describe, edit, label, annotate, etc)

create, replace, apply do not do this

There should not be any problem, apply just accept files. We could make same restriction to last-applied subcommands.

pwittrock · 2017-02-01T18:14:34Z

@adohe He Tony, I am assigning this to you for approval. Once you approve I plan to move forward with having it implemented.

@shiywang You will be implementing this

@jessfraz Would you shepherd through the new contributor process?

jessfraz · 2017-02-01T18:17:42Z

SGTM!

mengqiy · 2017-02-02T23:40:45Z

contributors/design-proposals/sig-cli/kubectl_apply_getsetdiff_last_applied_config.md

+kubectl apply diff last-applied -f deployment_nginx.yaml
+```
+
+Opens up a 2-way diff in the default diff viewer.  last-applied is old, local configuration is new.


This is not 3-way diff. Is this because we already have apply --dry-run -o json?
And is a 2-way diff helpful here? apply is not doing a 2-way diff.
If we only care about deletions, 2-way diff is OK.

My intention was that this was just a diff between the current and last applied config. It is meant to help users understand what they changed locally, but not show what was changed in the cluster.

I would like to see a different 3-way diff command for apply. Maybe kubectl apply diff. This is more complicated though.

I see. We should document this after implementing this.

2-way diff is ok here, we are actually calculate the deletion.

mengqiy · 2017-02-02T23:43:15Z

contributors/design-proposals/sig-cli/kubectl_apply_getsetdiff_last_applied_config.md

+1. Open the last-applied-config in an editor
+
+```sh
+kubectl apply edit last-applied -f deployment_nginx.yaml


last-applied-config lives in the annotation of the live obj. So we can support kubectl apply edit last-applied resource/name.

Yeah I like that.

mengqiy · 2017-02-02T23:49:47Z

contributors/design-proposals/sig-cli/kubectl_apply_getsetdiff_last_applied_config.md

+4. Remove the replicas field from the last-applied-config so it doesn't get deleted next apply
+  - `kubectl apply set last-applied -f deployment_nginx.yaml`
+5. Verify the last-applied-config has been updated
+  - `kubectl apply get last-applied -f deployment_nginx.yaml`


s/get/view/

yeah good idea

mengqiy · 2017-02-02T23:49:50Z

contributors/design-proposals/sig-cli/kubectl_apply_getsetdiff_last_applied_config.md

+3. Edit the last-applied-config and remove the replicas field
+  - `kubectl apply edit last-applied -f deployment_nginx.yaml`
+4. Verify the last-applied-config has been updated
+  - `kubectl apply get last-applied -f deployment_nginx.yaml`


s/get/view/

pwittrock · 2017-02-03T15:14:31Z

@shiywang made a few minor changes worth looking at. I think this is stable enough to begin implementing. Why don't you get started if you haven't already, and update the issue with the progress next week?

shiywang · 2017-02-03T15:20:57Z

@pwittrock, sounds good, but can I take a stab at part of this proposal first ? maybe one of a subcommand ?

shiywang · 2017-02-03T15:28:11Z

contributors/design-proposals/sig-cli/kubectl_apply_getsetdiff_last_applied_config.md

+
+## Naming and Format possibilities
+
+### Naming


those 6 names below are possibilities ? or we should keep them all ?

No, those were possibilities considered. We should only pick 1. I prefer the first one.

last-applied sounds good to me.

@pwittrock I prefer the first one too, I think we should remove this possibilities section from the final proposal, wdyt?

pwittrock · 2017-02-03T15:55:39Z

@shiywang Yes. You should start with kubectl apply view last-applied

The following should be supported:

kubectl apply view last-applied -f filename.yaml
kubectl apply view last-applied kind/name

shiywang · 2017-02-08T07:30:29Z

@pwittrock kubectl apply view last-applied is almost done, will open another pr to address apply set.

pwittrock · 2017-02-09T16:02:00Z

Thanks for the feedback Tony. Tried to include the answers to your questions in the updated doc. PTAL

pwittrock · 2017-02-09T16:23:52Z

Next week I am going to try to write 6 more design proposals that I intend to cover the majority of our usability gaps.

janetkuo · 2017-02-09T18:41:58Z

contributors/design-proposals/sig-cli/kubectl_apply_getsetdiff_last_applied_config.md

+
+1. Apply subcommands
+  - `kubectl apply set/view/diff last-applied
+  - a little bit odd to have 2 verbs in a row


It's also a bit odd to "view" something with kubectl apply, which is supposed to update (not view) resources.

That aside, are we planning to add more sub-commands to kubectl apply view? If not, should we combine view and last-applied?

I originally commented in kubernetes/kubernetes#41146 (review)

Such as kubectl apply view-last-applied -f dir/? That would work for me.

Was that what you were thinking Janet? If so I will update the doc.

FYI, this is my reason for kubectl apply view-last-applied instead of kubectl view last-applied. Interested in your thoughts.

My reasoning for organizing the commands like this is that it helps discoverability to put related commands closely together in the command tree. As a user, I probably want to discover all the commands related to apply and last-applied-configuration in one place, e.g. kubectl apply -h should print all the sub commands related to apply. Spreading the apply related commands across the tree will make it hard to discover new commands as well as pollute the other command namespaces.

Yes, something like kubectl apply view-last-applied was what I was thinking.

In kubernetes/kubernetes#41146 I originally suggested adding this feature in kubectl get or kubectl describe (maybe with a flag), since this command allows users to view resources instead of mutating them. However, after reading this proposal, I agree that discoverability is probably more important for this specific feature. Maybe just documenting "It's also a bit odd to view something with kubectl apply, which is supposed to update (not view) resources" in the proposal is enough (showing that we're aware of this but choose discoverability over this.)

…-config easier

pwittrock · 2017-02-10T23:12:31Z

@janetkuo updated with your feedback.

@adohe Are we good to merge this?

adohe-zz · 2017-02-11T08:18:45Z

Next week I am going to try to write 6 more design proposals that I intend to cover the majority of >our usability gaps.

Really look forward to this. 👍

adohe-zz · 2017-02-11T08:19:25Z

@pwittrock yes, LGTM. I am going to merge this.

adohe-zz · 2017-02-11T08:20:49Z

@shiywang finally we get this proposal approved, please take a look since you have started working on this. :)

shiywang · 2017-02-11T10:58:59Z

@adohe ok,this is great!!! thank you all 👍

shiywang · 2017-02-18T16:35:17Z

@pwittrock set-last-applied is ready for review, will be working on edit-last-applied

@pwittrock

Automatic merge from submit-queue (batch tested with PRs 41146, 41486, 41482, 41538, 41784) Add apply view-last-applied subcommand reopen pr #40984, implement part of kubernetes/community#287 for now unit test all pass, the output looks like: ```console shiywang@dhcp-140-33 template $ ./kubectl apply view last-applied deployment nginx-deployment apiVersion: extensions/v1beta1 kind: Deployment metadata: creationTimestamp: null name: nginx-deployment spec: strategy: {} template: metadata: creationTimestamp: null labels: app: nginx spec: containers: - image: nginx:1.12.10 name: nginx ports: - containerPort: 80 resources: {} status: {} ``` ```release-note Support new kubectl apply view-last-applied command for viewing the last configuration file applied ``` not sure if there is any flag I should updated or the some error handling I should changed. will generate docs when you guys think is ok. cc @pwittrock @jessfraz @adohe @ymqytw

@adohe

Automatic merge from submit-queue (batch tested with PRs 42044, 41694, 41927, 42050, 41987) Add apply set-last-applied subcommand implement part of kubernetes/community#287, will rebase after #41699 got merged, EDIT: since bug output format has been confirmed, will update the behavior of output format soon cc @kubernetes/sig-cli-pr-reviews @adohe @pwittrock ```release-note Support kubectl apply set-last-applied command to update the applied-applied-configuration annotation ```

Proposal for porcelain commands to make working with the last-applied…

@pwittrock

Automatic merge from submit-queue (batch tested with PRs 42256, 46479, 45436, 46440, 46417) Add `kubectl apply edit-last-applied` subcommand third command of kubernetes/community#287 Fixes kubernetes#44905 @pwittrock @adohe @ymqytw @kubernetes/sig-cli-feature-requests could you guys have an early review ? cause some of feature I'm not sure about, will add unit tests if you think it's ok.

Automatic merge from submit-queue (batch tested with PRs 53051, 52489, 53920). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Implement `kubectl alpha diff` to diff resources `kubectl alpha diff` lets you diff your resources against live resources, or last applied, or even preview what changes are going to be applied on the cluster. This is still quite premature, and mostly untested. **What this PR does / why we need it**: **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: Clearly not ready for Release note. ```release-note NONE ``` kubernetes/community#287

Proposal for porcelain commands to make working with the last-applied…

* Add self to the TOC (it's that easy!). * And fix Louis' avatar

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2017

mengqiy mentioned this pull request Jan 27, 2017

Stop edit from updating last-applied-config annotation and let replace respect --save-config kubernetes/kubernetes#40626

Closed

liggitt reviewed Jan 30, 2017

View reviewed changes

pwittrock assigned adohe-zz and mengqiy Feb 1, 2017

mengqiy reviewed Feb 2, 2017

View reviewed changes

shiywang reviewed Feb 3, 2017

View reviewed changes

shiywang mentioned this pull request Feb 5, 2017

Add apply view subcommand kubernetes/kubernetes#40984

Closed

shiywang mentioned this pull request Feb 8, 2017

Add apply view-last-applied subcommand kubernetes/kubernetes#41146

Merged

pwittrock force-pushed the master branch from 7d23f71 to 5facfb5 Compare February 9, 2017 16:22

janetkuo reviewed Feb 9, 2017

View reviewed changes

Proposal for porcelain commands to make working with the last-applied…

572c1e0

…-config easier

pwittrock force-pushed the master branch from 5facfb5 to 572c1e0 Compare February 10, 2017 23:13

adohe-zz merged commit 26d03a0 into kubernetes:master Feb 11, 2017

shiywang mentioned this pull request Feb 18, 2017

Add apply set-last-applied subcommand kubernetes/kubernetes#41694

Merged

shiywang mentioned this pull request Feb 28, 2017

Add kubectl apply edit-last-applied subcommand kubernetes/kubernetes#42256

Merged

shiywang mentioned this pull request Apr 4, 2017

[WIP] Add apply diff-last-applied subcommand kubernetes/kubernetes#44034

Closed

shiywang mentioned this pull request Apr 17, 2017

[WIP] Add apply diff-last-applied subcommand kubernetes/kubernetes#44544

Closed

ruebenramirez pushed a commit to ruebenramirez/community that referenced this pull request Apr 22, 2017

Merge pull request kubernetes#287 from pwittrock/master

377f6df

Proposal for porcelain commands to make working with the last-applied…

shiywang mentioned this pull request May 27, 2017

Add kubectl apply diff-last-applied subcommand kubernetes/kubernetes#46560

Closed

shiywang mentioned this pull request Jul 19, 2017

add kubectl apply diff-last-applied subcommand kubernetes/kubernetes#49174

Closed

This was referenced Oct 17, 2017

add kubectl apply diff-last-applied subcommand kubernetes/kubernetes#54064

Closed

add kubectl apply diff-last-applied subcommand kubernetes/kubernetes#54065

Closed

add kubectl apply diff-last-applied subcommand kubernetes/kubernetes#54067

Closed

apelisse mentioned this pull request Oct 25, 2017

Implement kubectl alpha diff to diff resources kubernetes/kubernetes#53920

Merged

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#287 from pwittrock/master

4296fb4

Proposal for porcelain commands to make working with the last-applied…

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

Add Josh to the TOC (kubernetes#287)

20e7456

* Add self to the TOC (it's that easy!). * And fix Louis' avatar

Proposal for porcelain commands to make working with the last-applied… #287

Proposal for porcelain commands to make working with the last-applied… #287

Conversation

pwittrock commented Jan 25, 2017

pwittrock commented Jan 25, 2017

mengqiy commented Jan 26, 2017

liggitt commented Jan 26, 2017

mengqiy commented Jan 26, 2017

pwittrock commented Jan 26, 2017

pwittrock commented Jan 26, 2017 • edited Loading

thockin commented Jan 27, 2017

pwittrock commented Jan 27, 2017

pwittrock commented Jan 27, 2017

liggitt commented Jan 27, 2017

mengqiy commented Jan 27, 2017 • edited Loading

mengqiy commented Jan 27, 2017

pwittrock commented Jan 27, 2017 • edited Loading

pwittrock commented Jan 27, 2017

pwittrock commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Feb 1, 2017

jessfraz commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Feb 3, 2017

shiywang commented Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Feb 3, 2017

shiywang commented Feb 8, 2017

pwittrock commented Feb 9, 2017

pwittrock commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Feb 10, 2017

adohe-zz commented Feb 11, 2017 • edited Loading

adohe-zz commented Feb 11, 2017

adohe-zz commented Feb 11, 2017

shiywang commented Feb 11, 2017

shiywang commented Feb 18, 2017 • edited Loading

pwittrock commented Jan 26, 2017 •

edited

Loading

mengqiy commented Jan 27, 2017 •

edited

Loading

pwittrock commented Jan 27, 2017 •

edited

Loading

shiywang commented Feb 3, 2017 •

edited

Loading

adohe-zz commented Feb 11, 2017 •

edited

Loading

shiywang commented Feb 18, 2017 •

edited

Loading