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 an edit command #1758

Merged
merged 3 commits into from
Apr 17, 2015
Merged

Conversation

smarterclayton
Copy link
Contributor

osc edit allows any resource on the server to be edited directly from
the CLI, which greatly simplifies the process of making changes to objects.
The edited object will be the API object in YAML format by default.

The behavior of this command is extremely similar to git commit or git rebase.

Examples:

$ osc edit svc/kubernetes
# in vim, add a label to kubernetes service

$ osc edit dc/mydeploymentconfig
# change the image, triggers a deployment automatically

$ osc edit -f <previously saved version of a resource>
# could update the resource version via 3 way diff

Errors will be captured to the comment at the top of the file and then
output to the user. Validation errors will allow the user to attempt
to edit the file a second time.

@jwforres review the experience, @deads2k the code

@smarterclayton
Copy link
Contributor Author

@bgrant0607 after having done this (even with known limitations) this seems like something that would ease a lot of simple use cases in Kube, because you can edit labels, fields, and other structures quickly. It would be better if we had an annotating YAML encoder (where we can stitch both field descriptions and error locations alongside the output field, as well as include example sections for code that users don't specify), but it's not strictly necessary.

@smarterclayton
Copy link
Contributor Author

Future features

  • 3 way diff on the CLI to tell users what has changed and reapply patches after an edit
  • Make it easier to recover from conflicts (need change detection so we know whether our patch applies cleanly on top of server changes).

for {
// try conversion to all the possible versions
// TODO: simplify by adding a ResourceBuilder mode
defaultVersion := clientConfig.Version
Copy link
Contributor

Choose a reason for hiding this comment

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

The defaultVersion is always the same. Move outside the loop to make that clear.

@bgrant0607
Copy link

I like it. I'd love to see this in kubectl.

It does a GET, allows the user to edit, and then a PUT? We need to put some energy into cleaning up objects in order to generate clean configs: scrub application of omitempty, convert more fields to pointers so we can distinguish unspecified from language defaults, strip out status, etc.

I agree that this could also work well with the config 3-way diff in the case that the edit were of a local config file rather than directly of the server object.

@smarterclayton
Copy link
Contributor Author

On Apr 16, 2015, at 10:08 AM, Brian Grant notifications@github.com wrote:

I like it. I'd love to see this in kubectl.

It does a GET, allows the user to edit, and then a PUT?

Yes, very similar code path to get (same arguments)
We need to put some energy into cleaning up objects in order to generate clean configs: scrub application of omitempty, convert more fields to pointers so we can distinguish unspecified from language defaults, strip out status, etc.

One advantage of null over omitempty is you know something is there. But in general smaller is better. I would like to use this to focus on swagger becoming usable for indicating behavior (null status because user can't edit that, etc)
I agree that this could also work well with the config 3-way diff in the case that the edit were of a local config file rather than directly of the server object.


Reply to this email directly or view it on GitHub.

@bgrant0607
Copy link

Re. null vs. omitempty: Could we use swagger to generate a fully populated spec? By default, we could present the stripped-down spec, but could provide a --full flag or somesuch for the full spec. We could present commonly used fields by default when creating a new object (create --edit), but also provide --full in that case.

@smarterclayton
Copy link
Contributor Author

Yeah probably. We need more flexibility in our output encoding to do that - the biggest thing I would want is annotations being added to a YAML stream so you could add inline comments, and then next to that it would be the decision to omit empty or continue.

----- Original Message -----

Re. null vs. omitempty: Could we use swagger to generate a fully populated
spec? By default, we could present the stripped-down spec, but could provide
a --full flag or somesuch for the full spec. We could present commonly used
fields by default when creating a new object (create --edit), but also
provide --full in that case.


Reply to this email directly or view it on GitHub:
#1758 (comment)

if err != nil {
return preservedFile(err, file, cmd.Out())
}
if bytes.Equal(original, edited) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I edit improperly and end up back in this loop, then the second time I want to exit and rethink my life. This deletes my file so I can't review.

@deads2k
Copy link
Contributor

deads2k commented Apr 16, 2015

Minor comments. I like it.

@smarterclayton
Copy link
Contributor Author

Updated, ready for review. Conflict should be separate. See the upstream changes which I'll land today or something.

----- Original Message -----

Minor comments. I like it.


Reply to this email directly or view it on GitHub:
#1758 (comment)

@deads2k
Copy link
Contributor

deads2k commented Apr 16, 2015

Any chance of getting this name updated: #1758 (comment)? It's not obvious from a quick scan that the meaning of info suddenly changed to something different, but related.

Otherwise, lgtm.

@smarterclayton
Copy link
Contributor Author

Fixed

----- Original Message -----

Any chance of getting this name updated:
#1758 (comment)? It's
not obvious from a quick scan that the meaning of info suddenly changed to
something different, but related.

Otherwise, lgtm.


Reply to this email directly or view it on GitHub:
#1758 (comment)

`osc edit` allows any resource on the server to be edited directly from
the CLI, which greatly simplifies the process of making changes to objects.
The edited object will be the API object in YAML format by default.

Errors will be captured to the comment at the top of the file and then
output to the user. Validation errors will allow the user to attempt
to edit the file a second time.
Should allow piping input streams to edit.
@smarterclayton
Copy link
Contributor Author

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1598/) (Image: devenv-fedora_1291)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to dde93d1

openshift-bot pushed a commit that referenced this pull request Apr 17, 2015
@openshift-bot openshift-bot merged commit f5675ba into openshift:master Apr 17, 2015
@smarterclayton smarterclayton modified the milestone: 0.5.0 (beta3) Apr 23, 2015
@smarterclayton smarterclayton deleted the add_an_edit branch May 18, 2015 02:16
jboyd01 pushed a commit to jboyd01/origin that referenced this pull request Mar 26, 2018
…service-catalog/' changes from c3e3071633..231772fcc0

231772fcc0 origin build: add origin tooling
98af588 v0.1.11 release changes
01e2f90 v0.1.10 release changes
49af948 clear polling queue before starting new operation (openshift#1855)
252958e Refactor common serviceclass validations (openshift#1858)
68f55c6 Catalog Controller should listen on https and serve metrics over TLS secured channel (openshift#1851)
5d0f773 Refactor common broker validations (openshift#1865)
eeaf285 Add NamespacedServiceBroker switch to helm chart (openshift#1864)
d2c960c Add NamespacedServiceBroker Feature (openshift#1863)
ef15310 Fix NamespaceScoped doc text for ns types (openshift#1862)
8d0a637 fix async deprovision retry (openshift#1832)
a918a16 Update registry code from serviceclass to clusterserviceclass (openshift#1852)
4dfd13c Bump dependency on go-open-service-broker-client to 0.0.6 (openshift#1856)
958b7cd Rename SharedServicePlanSpec to CommonServicePlanSpec (openshift#1850)
426aec3 pick a better random port to listen on in integration tests (openshift#1844)
6a59ada OrphanMitigation condition and different handling of retry timeout (openshift#1789)
c9b8f60 Extracting common broker spec elements into embeddable struct (openshift#1841)
3f8fab6 Extract common service class spec fields (openshift#1834)
93dab13 Support for OSB [PR#452](openservicebrokerapi/servicebroker#452). (openshift#1849)
5e1e90d [WIP] Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1847)
74f73c0 disable tests for deployment stage (openshift#1845)
82fc6e4 Revert "Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803)" (openshift#1843)
94b5795 gitignore integration.test binary (openshift#1840)
014c468 Extracting common plan spec into embeddable struct (openshift#1833)
5d7041b Use k8s NewUUID method exclusively  (openshift#1836)
eac3f96 A new test was added after prechecks happened for last pr. (openshift#1838)
4b5d159 Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803)
cc02f0e [svcat] Adding a filter to get plan. (openshift#1758)
70afb56 reset RemovedFromBroker flag on plans that are re-added by broker (openshift#1824)
712dd4a Add behavior to print deleted instance name (openshift#1806)
55505be Update catalog charts README configuration (openshift#1823)
6426c98 Controller reconciliation rework - part 2 (ServiceBinding) (openshift#1819)
c606560 Integrate svcat docs with Service Catalog's (openshift#1784)
e9aeeb0 Synchronize some generated code that was missed along the way (openshift#1801)
a63ebf7 Fix failing test: TestReconcileServiceInstanceWithFailedCondition (openshift#1813)
07ef743 Controller reconciliation rework - part 1 (ServiceInstance) (openshift#1779)
a7d602b Export the touch instance command (openshift#1809)
bddb9a7 Allow retries for instances with Failed condition after spec changes (openshift#1751)
a777af5 Add enhanced parameter options to provision (openshift#1785)
fd1a0b9 Print deleted bindings (openshift#1799)
36d437a Adding the ability to sync a service instance (openshift#1762)
1f60676 Remove Failed condition if there was no terminal failure (openshift#1788)
cd831de allow brokers to respond to getCatalog() with no services (openshift#1772) (openshift#1781)
e5c37ad Add ObservedGeneration and Provisioned into ServiceInstanceStatus (openshift#1748)
9021d8b Add carolynvs to OWNERS (openshift#1780)
b7643d6 Add all-namespaces flag to svcat (openshift#1782)
01e652f Use docker to interact with files made by docker (openshift#1777)
REVERT: c3e3071633 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: 231772fcc00be08b6b2665a39c4a3bacb0b2271f
jboyd01 pushed a commit to jboyd01/origin that referenced this pull request Mar 27, 2018
…service-catalog/' changes from c3e3071633..231772fcc0

231772fcc0 origin build: add origin tooling
98af588 v0.1.11 release changes
01e2f90 v0.1.10 release changes
49af948 clear polling queue before starting new operation (openshift#1855)
252958e Refactor common serviceclass validations (openshift#1858)
68f55c6 Catalog Controller should listen on https and serve metrics over TLS secured channel (openshift#1851)
5d0f773 Refactor common broker validations (openshift#1865)
eeaf285 Add NamespacedServiceBroker switch to helm chart (openshift#1864)
d2c960c Add NamespacedServiceBroker Feature (openshift#1863)
ef15310 Fix NamespaceScoped doc text for ns types (openshift#1862)
8d0a637 fix async deprovision retry (openshift#1832)
a918a16 Update registry code from serviceclass to clusterserviceclass (openshift#1852)
4dfd13c Bump dependency on go-open-service-broker-client to 0.0.6 (openshift#1856)
958b7cd Rename SharedServicePlanSpec to CommonServicePlanSpec (openshift#1850)
426aec3 pick a better random port to listen on in integration tests (openshift#1844)
6a59ada OrphanMitigation condition and different handling of retry timeout (openshift#1789)
c9b8f60 Extracting common broker spec elements into embeddable struct (openshift#1841)
3f8fab6 Extract common service class spec fields (openshift#1834)
93dab13 Support for OSB [PR#452](openservicebrokerapi/servicebroker#452). (openshift#1849)
5e1e90d [WIP] Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1847)
74f73c0 disable tests for deployment stage (openshift#1845)
82fc6e4 Revert "Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803)" (openshift#1843)
94b5795 gitignore integration.test binary (openshift#1840)
014c468 Extracting common plan spec into embeddable struct (openshift#1833)
5d7041b Use k8s NewUUID method exclusively  (openshift#1836)
eac3f96 A new test was added after prechecks happened for last pr. (openshift#1838)
4b5d159 Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803)
cc02f0e [svcat] Adding a filter to get plan. (openshift#1758)
70afb56 reset RemovedFromBroker flag on plans that are re-added by broker (openshift#1824)
712dd4a Add behavior to print deleted instance name (openshift#1806)
55505be Update catalog charts README configuration (openshift#1823)
6426c98 Controller reconciliation rework - part 2 (ServiceBinding) (openshift#1819)
c606560 Integrate svcat docs with Service Catalog's (openshift#1784)
e9aeeb0 Synchronize some generated code that was missed along the way (openshift#1801)
a63ebf7 Fix failing test: TestReconcileServiceInstanceWithFailedCondition (openshift#1813)
07ef743 Controller reconciliation rework - part 1 (ServiceInstance) (openshift#1779)
a7d602b Export the touch instance command (openshift#1809)
bddb9a7 Allow retries for instances with Failed condition after spec changes (openshift#1751)
a777af5 Add enhanced parameter options to provision (openshift#1785)
fd1a0b9 Print deleted bindings (openshift#1799)
36d437a Adding the ability to sync a service instance (openshift#1762)
1f60676 Remove Failed condition if there was no terminal failure (openshift#1788)
cd831de allow brokers to respond to getCatalog() with no services (openshift#1772) (openshift#1781)
e5c37ad Add ObservedGeneration and Provisioned into ServiceInstanceStatus (openshift#1748)
9021d8b Add carolynvs to OWNERS (openshift#1780)
b7643d6 Add all-namespaces flag to svcat (openshift#1782)
01e652f Use docker to interact with files made by docker (openshift#1777)
REVERT: c3e3071633 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: 231772fcc00be08b6b2665a39c4a3bacb0b2271f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants