fix: kubectl edit apply twice does not work

Fixes #4797
kubernetes-sigs · Jun 6, 2020 · 7a9da55 · 7a9da55
1 parent f716cc8
commit 7a9da55
Show file tree

Hide file tree

Showing 10 changed files with 113 additions and 84 deletions.
diff --git a/packages/core/src/models/execOptions.ts b/packages/core/src/models/execOptions.ts
@@ -28,7 +28,7 @@ export interface ExecOptions {
   execUUID?: string
 
   /** pass through uninterpreted data */
-  data?: string | Buffer
+  data?: string | Buffer | Record<string, any>
 
   /** environment variable map */
   env?: Record<string, string>

diff --git a/packages/core/src/models/mmr/content-types.ts b/packages/core/src/models/mmr/content-types.ts
@@ -21,7 +21,7 @@ import { Table, isTable } from '../../webapp/models/table'
 import { Entity, MetadataBearing } from '../entity'
 import { isHTML } from '../../util/types'
 import { ModeOrButton, Button } from './types'
-import { ToolbarText, ToolbarAlert } from '../../webapp/views/toolbar-text'
+import { ToolbarText } from '../../webapp/views/toolbar-text'
 
 /**
  * A `ScalarResource` is Any kind of resource that is directly
@@ -35,12 +35,7 @@ export interface ScalarContent<T = ScalarResource> {
 }
 
 export type ToolbarProps = {
-  willUpdateToolbar?: (
-    toolbarText: ToolbarText,
-    extraButtons?: Button[],
-    extraButtonsOverride?: boolean,
-    alerts?: ToolbarAlert[]
-  ) => void
+  willUpdateToolbar?: (toolbarText: ToolbarText, extraButtons?: Button[], extraButtonsOverride?: boolean) => void
 }
 export type ReactProvider = { react: (props: ToolbarProps) => ReactElement<any> }
 export function isReactProvider(entity: ScalarLike<MetadataBearing>): entity is ReactProvider {

diff --git a/packages/core/src/webapp/views/toolbar-text.ts b/packages/core/src/webapp/views/toolbar-text.ts
@@ -23,13 +23,14 @@ type ToolbarTextType = 'info' | 'success' | 'warning' | 'error'
 
 type ToolbarTextValue = string | Element
 
-export interface ToolbarText {
-  type: ToolbarTextType
-  text: ToolbarTextValue
-}
-
 export interface ToolbarAlert {
   type: ToolbarTextType
   title: string
   body?: string
 }
+
+export interface ToolbarText {
+  type: ToolbarTextType
+  text: ToolbarTextValue
+  alerts?: ToolbarAlert[] /* auto destruct */
+}
diff --git a/packages/test/src/api/keys.ts b/packages/test/src/api/keys.ts
@@ -5,6 +5,7 @@ export const keys = {
   PageDown: '\uE00F',
   End: '\uE010',
   Home: '\uE011',
+  Delete: '\uE017',
   BACKSPACE: '\uE003',
   TAB: '\uE004',
   ENTER: '\uE007',

diff --git a/plugins/plugin-client-common/src/components/Content/Editor/index.tsx b/plugins/plugin-client-common/src/components/Content/Editor/index.tsx
@@ -19,16 +19,7 @@ import { extname } from 'path'
 import { IDisposable, editor as Monaco, Range } from 'monaco-editor'
 
 import { File, isFile } from '@kui-shell/plugin-bash-like/fs'
-import {
-  Button,
-  REPL,
-  StringContent,
-  ToolbarText,
-  ToolbarProps,
-  ToolbarAlert,
-  MultiModalResponse,
-  i18n
-} from '@kui-shell/core'
+import { Button, REPL, StringContent, ToolbarText, ToolbarProps, MultiModalResponse, i18n } from '@kui-shell/core'
 
 import ClearButton from './ClearButton'
 import SaveFileButton from './SaveFileButton'
@@ -47,7 +38,7 @@ interface WithOptions {
     clearable?: boolean
     save?: {
       label: string
-      onSave: (data: string) => Promise<void | ToolbarAlert>
+      onSave: (data: string) => Promise<void | { noToolbarUpdate?: boolean; toolbarText: ToolbarText }>
     }
     revert?: {
       label: string
@@ -175,17 +166,27 @@ export default class Editor extends React.PureComponent<Props, State> {
           kind: 'view' as const,
           command: async () => {
             try {
-              const onSavedText = await onSave(editor.getValue())
-              props.willUpdateToolbar(
-                this.allClean(props),
-                !clearable ? undefined : [ClearButton(editor)],
-                undefined,
-                onSavedText ? [onSavedText] : undefined
-              )
+              const save = await onSave(editor.getValue())
+              if (!(save && save.noToolbarUpdate)) {
+                props.willUpdateToolbar(
+                  (save && save.toolbarText) || this.allClean(props),
+                  !clearable ? undefined : [ClearButton(editor)]
+                )
+              }
             } catch (err) {
-              props.willUpdateToolbar({ type: 'warning', text: strings('isModified') }, undefined, undefined, [
-                { type: 'error', title: strings('errorApplying'), body: err.message }
-              ])
+              const alert = {
+                type: 'warning' as const,
+                text: strings('isModified'),
+                alerts: [
+                  {
+                    type: 'error' as const,
+                    title: strings('errorApplying'),
+                    body: err.message
+                  }
+                ]
+              }
+
+              props.willUpdateToolbar(alert, undefined, undefined)
             }
           }
         })

diff --git a/plugins/plugin-client-common/src/components/Views/Sidecar/Toolbar.tsx b/plugins/plugin-client-common/src/components/Views/Sidecar/Toolbar.tsx
@@ -26,6 +26,7 @@ export type Props = {
   buttons: Button[]
   response: MultiModalResponse
   toolbarText?: ToolbarText
+  noAlerts?: boolean
   args: {
     argvNoOptions: string[]
     parsedOptions: ParsedOptions

diff --git a/plugins/plugin-client-common/src/components/Views/Sidecar/ToolbarContainer.tsx b/plugins/plugin-client-common/src/components/Views/Sidecar/ToolbarContainer.tsx
@@ -40,13 +40,8 @@ export default class ToolbarContainer extends React.PureComponent<Props, State>
   }
 
   /** Called by children if they desire an update to the Toolbar */
-  private onToolbarUpdate(
-    toolbarText: ToolbarText,
-    extraButtons?: Button[],
-    extraButtonsOverride?: boolean,
-    alerts?: ToolbarAlert[]
-  ) {
-    this.setState({ toolbarText, extraButtons, extraButtonsOverride, alerts })
+  private onToolbarUpdate(toolbarText: ToolbarText, extraButtons?: Button[], extraButtonsOverride?: boolean) {
+    this.setState({ toolbarText, extraButtons, extraButtonsOverride })
   }
 
   /** Graft on the toolbar event management */
@@ -65,6 +60,11 @@ export default class ToolbarContainer extends React.PureComponent<Props, State>
         ? this.state.extraButtons
         : this.props.buttons.concat(this.state.extraButtons || [])
     const toolbarHasContent = this.state.toolbarText || toolbarButtons.length !== 0
+    const toolbarHasAlerts =
+      !this.props.noAlerts &&
+      this.state.toolbarText &&
+      this.state.toolbarText.alerts &&
+      this.state.toolbarText.alerts.length !== 0
 
     return (
       <div className="full-height">
@@ -77,7 +77,7 @@ export default class ToolbarContainer extends React.PureComponent<Props, State>
             buttons={toolbarButtons}
           />
         )}
-        {this.state.alerts && this.state.alerts.map((alert, id) => <Alert key={id} alert={alert} />)}
+        {toolbarHasAlerts && this.state.toolbarText.alerts.map((alert, id) => <Alert key={id} alert={alert} />)}
         <React.Suspense fallback={<div />}>{this.children()}</React.Suspense>
       </div>
     )

diff --git a/plugins/plugin-client-common/src/components/Views/Sidecar/TopNavSidecar.tsx b/plugins/plugin-client-common/src/components/Views/Sidecar/TopNavSidecar.tsx
@@ -56,6 +56,7 @@ interface HistoryEntry extends SidecarHistoryEntry {
   buttons: Button[]
   tabs: Readonly<MultiModalMode[]>
   response: Readonly<MultiModalResponse>
+  defaultMode: number
 }
 
 export function getStateFromMMR(
@@ -102,6 +103,7 @@ export function getStateFromMMR(
     argvNoOptions,
     parsedOptions,
     currentTabIndex: defaultMode,
+    defaultMode,
     tabs,
     buttons,
     response
@@ -270,6 +272,7 @@ export default class TopNavSidecar extends BaseSidecar<MultiModalResponse, Histo
             response={this.current.response}
             args={{ argvNoOptions: this.state.current.argvNoOptions, parsedOptions: this.state.current.parsedOptions }}
             toolbarText={toolbarText}
+            noAlerts={this.current.currentTabIndex !== this.state.current.defaultMode}
             buttons={this.current.buttons}
           >
             {this.bodyContent(idx)}

diff --git a/plugins/plugin-kubectl/src/controller/kubectl/edit.ts b/plugins/plugin-kubectl/src/controller/kubectl/edit.ts
@@ -15,7 +15,7 @@
  */
 
 import { v4 as uuid } from 'uuid'
-import { Arguments, MultiModalResponse, Registrar, ToolbarAlert, i18n } from '@kui-shell/core'
+import { Arguments, MultiModalResponse, Registrar, ExecOptions, i18n } from '@kui-shell/core'
 
 import flags from './flags'
 import { doExecWithStdout } from './exec'
@@ -33,7 +33,7 @@ interface EditableSpec {
   clearable: boolean
   save: {
     label: string
-    onSave(data: string): ToolbarAlert | Promise<ToolbarAlert>
+    onSave(data: string): Promise<void | { noToolbarUpdate: boolean }>
   }
   revert: {
     onRevert(): string | Promise<string>
@@ -112,9 +112,15 @@ export function editSpec(
           }
         })
 
+        // to show the updated resource after apply,
+        // we re-execute the original edit command after applying the changes.
+        // `partOfApply` here is used to signify this execution is part of a chain of controller
+        await args.REPL.pexec(args.command, { echo: false, data: { partOfApply: true } })
+
         return {
-          type: 'success' as const,
-          title: strings('Successfully Applied')
+          // disable editor's auto toolbar update,
+          // since this command will handle the toolbarText by itself
+          noToolbarUpdate: true
         }
       }
     },
@@ -182,12 +188,33 @@ export async function doEdit(cmd: string, args: Arguments<KubeOptions>) {
   }
 }
 
+interface EditAfterApply {
+  data: {
+    partOfApply: boolean
+  }
+}
+
+function isEditAfterApply(options: ExecOptions): options is EditAfterApply {
+  const opts = options as EditAfterApply
+  return opts && opts.data && opts.data.partOfApply !== undefined
+}
+
 export async function editable(cmd: string, args: Arguments<KubeOptions>, response: KubeResource): Promise<Editable> {
   const spec = editSpec(cmd, response.metadata.namespace, args, response)
 
-  const view = Object.assign(await getView(args, response), {
-    spec: Object.assign(response.spec || {}, spec)
+  const baseView = await getView(args, response)
+
+  const view = Object.assign(baseView, {
+    spec: Object.assign(response.spec || {}, spec),
+    toolbarText: !isEditAfterApply(args.execOptions)
+      ? baseView.toolbarText
+      : {
+          type: baseView.toolbarText.type,
+          text: baseView.toolbarText.text,
+          alerts: [{ type: 'success', title: strings('Successfully Applied') }]
+        }
   })
+
   return view
 }
 

diff --git a/plugins/plugin-kubectl/src/test/k8s1/edit.ts b/plugins/plugin-kubectl/src/test/k8s1/edit.ts
@@ -71,7 +71,7 @@ commands.forEach(command => {
       })
     }
 
-    const modifyWithError = (title: string, where: string, expectedError: string) => {
+    const modifyWithError = (title: string, where: string, expectedError: string, revert: false) => {
       it(`should modify the content, introducing a ${title}`, async () => {
         try {
           const actualText = await Util.getValueFromMonaco(this.app)
@@ -91,6 +91,17 @@ commands.forEach(command => {
 
           // an error state and the garbage text had better appear in the toolbar text
           await SidecarExpect.toolbarAlert({ type: 'error', text: expectedError || garbage, exact: false })(this.app)
+
+          if (revert) {
+            await this.app.client.click(lineSelector)
+            await new Promise(resolve => setTimeout(resolve, 2000))
+            await this.app.client.keys(
+              where === Keys.Home
+                ? `${where}${Keys.DELETE.repeat(garbage.length)}`
+                : `${where}${Keys.BACKSPACE.repeat(garbage.length)}`
+            )
+            await new Promise(resolve => setTimeout(resolve, 2000))
+          }
         } catch (err) {
           await Common.oops(this, true)(err)
         }
@@ -128,36 +139,7 @@ commands.forEach(command => {
         }
       })
 
-      it('should get the modified pod', async () => {
-        try {
-          const selector = await CLI.command(`${command} get pod ${name} ${inNamespace}`, this.app).then(
-            ReplExpect.okWithCustom({ selector: Selectors.BY_NAME(name) })
-          )
-
-          // wait for the badge to become green
-          await waitForGreen(this.app, selector)
-
-          // now click on the table row
-          await this.app.client.click(`${selector} .clickable`)
-          await SidecarExpect.open(this.app)
-            .then(SidecarExpect.mode(defaultModeForGet))
-            .then(SidecarExpect.showing(name))
-        } catch (err) {
-          return Common.oops(this, true)
-        }
-      })
-
-      it('should switch to yaml tab', async () => {
-        try {
-          await this.app.client.waitForVisible(Selectors.SIDECAR_MODE_BUTTON('raw'))
-          await this.app.client.click(Selectors.SIDECAR_MODE_BUTTON('raw'))
-          await this.app.client.waitForVisible(Selectors.SIDECAR_MODE_BUTTON_SELECTED('raw'))
-        } catch (err) {
-          await Common.oops(this, true)(err)
-        }
-      })
-
-      it('should show the modified content', () => {
+      it('should show the modified content in the current yaml tab', () => {
         return SidecarExpect.yaml({ metadata: { labels: { [key]: value } } })(this.app).catch(Common.oops(this, true))
       })
     }
@@ -204,7 +186,23 @@ commands.forEach(command => {
         }
       })
 
-      modify(name, 'foo2', 'bar2')
+      modify(name, 'clickfoo1', 'clickbar1')
+      modify(name, 'clickfoo2', 'clickbar2') // after success, should re-modify the resource in the current tab successfully
+      validationError(true) // do unsupported edits in the current tab, validate the error alert, and then undo the changes
+      modify(name, 'clickfoo3', 'clickbar3') // after error, should re-modify the resource in the current tab successfully
+
+      it('should switch to summary tab and see no alerts', async () => {
+        try {
+          await this.app.client.waitForVisible(Selectors.SIDECAR_MODE_BUTTON('summary'))
+          await this.app.client.click(Selectors.SIDECAR_MODE_BUTTON('summary'))
+          await this.app.client.waitForVisible(Selectors.SIDECAR_MODE_BUTTON_SELECTED('summary'))
+
+          // toolbar alert should not exist
+          await this.app.client.waitForExist(Selectors.SIDECAR_ALERT('success'), CLI.waitTimeout, true)
+        } catch (err) {
+          await Common.oops(this, true)(err)
+        }
+      })
     }
 
     //
@@ -221,12 +219,14 @@ commands.forEach(command => {
 
     edit(nginx)
     modify(nginx)
+    modify(nginx, 'foo1', 'bar1') // successfully re-modify the resource in the current tab
+    validationError(true) // do unsupported edits in the current tab, and then undo the changes
+    modify(nginx, 'foo2', 'bar2') // after error, successfully re-modify the resource in the current tab
+    parseError() // after sucess, do unsupported edits
 
     edit(nginx)
-    validationError()
-
-    edit(nginx)
-    parseError()
+    validationError(true) // do unsupported edits in the current tab, then undo the changes
+    parseError() // after error, do another unsupported edits in the current tab
 
     it('should refresh', () => Common.refresh(this))