From a4eb8a6a209d1a8d8ee70a417b33832638e7ca58 Mon Sep 17 00:00:00 2001 From: grobbie Date: Fri, 25 Feb 2022 23:14:19 +0100 Subject: [PATCH 1/6] format code with prettifier --- frontend/src/lib/Apis.ts | 3 +- frontend/src/pages/PipelineList.test.tsx | 77 ++++++++++++++++++++++++ frontend/src/pages/PipelineList.tsx | 20 ++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/frontend/src/lib/Apis.ts b/frontend/src/lib/Apis.ts index 91da1696911..dceb1f24b66 100644 --- a/frontend/src/lib/Apis.ts +++ b/frontend/src/lib/Apis.ts @@ -353,7 +353,8 @@ export class Apis { '/pipelines/upload_version', v1beta1Prefix, `name=${encodeURIComponent(versionName)}&pipelineid=${encodeURIComponent(pipelineId)}` + - (description ? `&description=${encodeURIComponent(description)}` : ''), + (description ? `&description=${encodeURIComponent(description)}` : '') + + (namespace ? `&namespace=${encodeURIComponent(namespace)}` : ''), { body: fd, cache: 'no-cache', diff --git a/frontend/src/pages/PipelineList.test.tsx b/frontend/src/pages/PipelineList.test.tsx index 8930bbf3976..7d8540acb8c 100644 --- a/frontend/src/pages/PipelineList.test.tsx +++ b/frontend/src/pages/PipelineList.test.tsx @@ -60,7 +60,24 @@ describe('PipelineList', () => { ); } +<<<<<<< HEAD async function mountWithNPipelines(n: number): Promise { +======= + function mockListNPipelines(n: number = 1) { + return () => + Promise.resolve({ + pipelines: range(n).map(i => ({ + id: 'test-pipeline-id' + i, + name: 'test pipeline name' + i, + })), + }); + } + + async function mountWithNPipelines( + n: number, + { namespace }: { namespace?: string } = {}, + ): Promise { +>>>>>>> bbe4ad0a6 (format code with prettifier) listPipelinesSpy.mockImplementation(() => ({ pipelines: range(n).map(i => ({ id: 'test-pipeline-id' + i, @@ -146,7 +163,18 @@ describe('PipelineList', () => { listPipelinesSpy.mockImplementationOnce(() => ({ pipelines: [{ name: 'pipeline1' }] })); tree = TestUtils.mountWithRouter(); await listPipelinesSpy; +<<<<<<< HEAD expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc', ''); +======= + expect(listPipelinesSpy).toHaveBeenLastCalledWith( + '', + 10, + 'created_at desc', + '', + undefined, + undefined, + ); +>>>>>>> bbe4ad0a6 (format code with prettifier) expect(tree.state()).toHaveProperty('displayPipelines', [ { expandState: 0, name: 'pipeline1' }, ]); @@ -160,7 +188,18 @@ describe('PipelineList', () => { expect(refreshBtn).toBeDefined(); await refreshBtn!.action(); expect(listPipelinesSpy.mock.calls.length).toBe(2); +<<<<<<< HEAD expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc', ''); +======= + expect(listPipelinesSpy).toHaveBeenLastCalledWith( + '', + 10, + 'created_at desc', + '', + undefined, + undefined, + ); +>>>>>>> bbe4ad0a6 (format code with prettifier) expect(updateBannerSpy).toHaveBeenLastCalledWith({}); }); @@ -186,7 +225,18 @@ describe('PipelineList', () => { TestUtils.makeErrorResponseOnce(listPipelinesSpy, 'bad stuff happened'); await refreshBtn!.action(); expect(listPipelinesSpy.mock.calls.length).toBe(2); +<<<<<<< HEAD expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc', ''); +======= + expect(listPipelinesSpy).toHaveBeenLastCalledWith( + '', + 10, + 'created_at desc', + '', + undefined, + undefined, + ); +>>>>>>> bbe4ad0a6 (format code with prettifier) expect(updateBannerSpy).toHaveBeenLastCalledWith( expect.objectContaining({ additionalInfo: 'bad stuff happened', @@ -562,4 +612,31 @@ describe('PipelineList', () => { open: true, }); }); +<<<<<<< HEAD +======= + + it("doesn't keep error message for request from previous namespace", async () => { + listPipelinesSpy.mockImplementation(() => Promise.reject('namespace cannot be empty')); + const { rerender } = render( + + + + + , + ); + + listPipelinesSpy.mockImplementation(mockListNPipelines()); + rerender( + + + + + , + ); + await act(TestUtils.flushPromises); + expect(updateBannerSpy).toHaveBeenLastCalledWith( + {}, // Empty object means banner has no error message + ); + }); +>>>>>>> bbe4ad0a6 (format code with prettifier) }); diff --git a/frontend/src/pages/PipelineList.tsx b/frontend/src/pages/PipelineList.tsx index f73606d898d..07735f867a8 100644 --- a/frontend/src/pages/PipelineList.tsx +++ b/frontend/src/pages/PipelineList.tsx @@ -247,8 +247,28 @@ class PipelineList extends Page<{}, PipelineListState> { try { method === ImportMethod.LOCAL +<<<<<<< HEAD ? await Apis.uploadPipeline(name, description || '', file!) : await Apis.pipelineServiceApi.createPipeline({ name, url: { pipeline_url: url } }); +======= + ? await Apis.uploadPipeline(name, description || '', file!, this.props.namespace) + : await Apis.pipelineServiceApi.createPipeline({ + name, + url: { pipeline_url: url }, + resource_references: this.props.namespace + ? [ + { + key: { + id: this.props.namespace, + type: ApiResourceType.NAMESPACE, + }, + relationship: ApiRelationship.OWNER, + }, + ] + : '', + }); + +>>>>>>> bbe4ad0a6 (format code with prettifier) this.setStateSafe({ uploadDialogOpen: false }); this.refresh(); return true; From 6925de9157f54dac6786e89848f386363936ecc3 Mon Sep 17 00:00:00 2001 From: grobbie Date: Sat, 26 Feb 2022 08:30:37 +0100 Subject: [PATCH 2/6] feat(frontend,sdk) towards namespaced pipelines #4197 - adapt swagger def and regenerate Go SDK - remove namespacing from pipeline_version - is implicit in Python SDK --- .../upload_pipeline_parameters.go | 29 +++++++++++++++++++ .../swagger/kfp_api_single_file.swagger.json | 6 ++++ .../api/swagger/pipeline.upload.swagger.json | 8 ++++- sdk/python/kfp/client/client.py | 7 ++++- 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/backend/api/go_http_client/pipeline_upload_client/pipeline_upload_service/upload_pipeline_parameters.go b/backend/api/go_http_client/pipeline_upload_client/pipeline_upload_service/upload_pipeline_parameters.go index 8b36f7ff7df..3945ae005a7 100644 --- a/backend/api/go_http_client/pipeline_upload_client/pipeline_upload_service/upload_pipeline_parameters.go +++ b/backend/api/go_http_client/pipeline_upload_client/pipeline_upload_service/upload_pipeline_parameters.go @@ -65,6 +65,8 @@ type UploadPipelineParams struct { Description *string /*Name*/ Name *string + /*Namespace*/ + Namespace *string /*Uploadfile The pipeline to upload. Maximum size of 32MB is supported. @@ -131,6 +133,17 @@ func (o *UploadPipelineParams) SetName(name *string) { o.Name = name } +// WithNamespace adds the namespace to the upload pipeline params +func (o *UploadPipelineParams) WithNamespace(namespace *string) *UploadPipelineParams { + o.SetNamespace(namespace) + return o +} + +// SetNamespace adds the namespace to the upload pipeline params +func (o *UploadPipelineParams) SetNamespace(namespace *string) { + o.Namespace = namespace +} + // WithUploadfile adds the uploadfile to the upload pipeline params func (o *UploadPipelineParams) WithUploadfile(uploadfile runtime.NamedReadCloser) *UploadPipelineParams { o.SetUploadfile(uploadfile) @@ -182,6 +195,22 @@ func (o *UploadPipelineParams) WriteToRequest(r runtime.ClientRequest, reg strfm } + if o.Namespace != nil { + + // query param namespace + var qrNamespace string + if o.Namespace != nil { + qrNamespace = *o.Namespace + } + qNamespace := qrNamespace + if qNamespace != "" { + if err := r.SetQueryParam("namespace", qNamespace); err != nil { + return err + } + } + + } + // form file param uploadfile if err := r.SetFileParam("uploadfile", o.Uploadfile); err != nil { return err diff --git a/backend/api/swagger/kfp_api_single_file.swagger.json b/backend/api/swagger/kfp_api_single_file.swagger.json index 3b70da6d817..bd675f66026 100644 --- a/backend/api/swagger/kfp_api_single_file.swagger.json +++ b/backend/api/swagger/kfp_api_single_file.swagger.json @@ -1406,6 +1406,12 @@ "in": "query", "required": false, "type": "string" + }, + { + "name": "namespace", + "in": "query", + "required": false, + "type": "string" } ], "tags": [ diff --git a/backend/api/swagger/pipeline.upload.swagger.json b/backend/api/swagger/pipeline.upload.swagger.json index 21ccb6d753f..02ca5c10c25 100644 --- a/backend/api/swagger/pipeline.upload.swagger.json +++ b/backend/api/swagger/pipeline.upload.swagger.json @@ -51,7 +51,13 @@ "in": "query", "required": false, "type": "string" - } + }, + { + "name": "namespace", + "in": "query", + "required": false, + "type": "string" + } ], "tags": [ "PipelineUploadService" diff --git a/sdk/python/kfp/client/client.py b/sdk/python/kfp/client/client.py index 54abfd8decf..928f2f02eea 100644 --- a/sdk/python/kfp/client/client.py +++ b/sdk/python/kfp/client/client.py @@ -1338,6 +1338,7 @@ def upload_pipeline( pipeline_package_path: str = None, pipeline_name: str = None, description: str = None, + namespace: str = None, ) -> kfp_server_api.ApiPipeline: """Uploads the pipeline to the Kubeflow Pipelines cluster. @@ -1352,7 +1353,7 @@ def upload_pipeline( """ response = self._upload_api.upload_pipeline( - pipeline_package_path, name=pipeline_name, description=description) + pipeline_package_path, name=pipeline_name, description=description, namespace=namespace) if self._is_ipython(): import IPython html = 'Pipeline details.' % ( @@ -1367,6 +1368,7 @@ def upload_pipeline_version( pipeline_id: Optional[str] = None, pipeline_name: Optional[str] = None, description: Optional[str] = None, + namespace: Optional[str] = None, ) -> kfp_server_api.ApiPipelineVersion: """Uploads a new version of the pipeline to the KFP cluster. @@ -1402,6 +1404,9 @@ def upload_pipeline_version( if description: kwargs['description'] = description + if namespace: + kwargs['namespace'] = namespace + response = self._upload_api.upload_pipeline_version( pipeline_package_path, **kwargs) From 51bbbfdcef6827e8086d9fc4a6474ee0f154b6a2 Mon Sep 17 00:00:00 2001 From: grobbie Date: Tue, 22 Mar 2022 15:27:57 +0100 Subject: [PATCH 3/6] prepare for rebase to master branch --- frontend/src/pages/PipelineList.test.tsx | 30 +++++++----------------- frontend/src/pages/PipelineList.tsx | 17 ++++++++------ 2 files changed, 18 insertions(+), 29 deletions(-) diff --git a/frontend/src/pages/PipelineList.test.tsx b/frontend/src/pages/PipelineList.test.tsx index 7d8540acb8c..c6f4883f6c7 100644 --- a/frontend/src/pages/PipelineList.test.tsx +++ b/frontend/src/pages/PipelineList.test.tsx @@ -20,9 +20,12 @@ import * as React from 'react'; import { RoutePage, RouteParams } from '../components/Router'; import { Apis } from '../lib/Apis'; import { ButtonKeys } from '../lib/Buttons'; +import { render, act } from '@testing-library/react'; import TestUtils from '../TestUtils'; import { PageProps } from './Page'; -import PipelineList from './PipelineList'; +import EnhancedPipelineList, { PipelineList } from './PipelineList'; +import { MemoryRouter } from 'react-router-dom'; +import { NamespaceContext } from 'src/lib/KubeflowClient'; describe('PipelineList', () => { let tree: ReactWrapper | ShallowWrapper; @@ -60,9 +63,6 @@ describe('PipelineList', () => { ); } -<<<<<<< HEAD - async function mountWithNPipelines(n: number): Promise { -======= function mockListNPipelines(n: number = 1) { return () => Promise.resolve({ @@ -77,7 +77,6 @@ describe('PipelineList', () => { n: number, { namespace }: { namespace?: string } = {}, ): Promise { ->>>>>>> bbe4ad0a6 (format code with prettifier) listPipelinesSpy.mockImplementation(() => ({ pipelines: range(n).map(i => ({ id: 'test-pipeline-id' + i, @@ -88,7 +87,7 @@ describe('PipelineList', () => { }, })), })); - tree = TestUtils.mountWithRouter(); + tree = TestUtils.mountWithRouter(); await listPipelinesSpy; await TestUtils.flushPromises(); tree.update(); // Make sure the tree is updated before returning it @@ -103,7 +102,9 @@ describe('PipelineList', () => { afterEach(async () => { // unmount() should be called before resetAllMocks() in case any part of the unmount life cycle // depends on mocks/spies - await tree.unmount(); + if (tree.exists()) { + await tree.unmount(); + } jest.restoreAllMocks(); }); @@ -163,9 +164,6 @@ describe('PipelineList', () => { listPipelinesSpy.mockImplementationOnce(() => ({ pipelines: [{ name: 'pipeline1' }] })); tree = TestUtils.mountWithRouter(); await listPipelinesSpy; -<<<<<<< HEAD - expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc', ''); -======= expect(listPipelinesSpy).toHaveBeenLastCalledWith( '', 10, @@ -174,7 +172,6 @@ describe('PipelineList', () => { undefined, undefined, ); ->>>>>>> bbe4ad0a6 (format code with prettifier) expect(tree.state()).toHaveProperty('displayPipelines', [ { expandState: 0, name: 'pipeline1' }, ]); @@ -188,9 +185,6 @@ describe('PipelineList', () => { expect(refreshBtn).toBeDefined(); await refreshBtn!.action(); expect(listPipelinesSpy.mock.calls.length).toBe(2); -<<<<<<< HEAD - expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc', ''); -======= expect(listPipelinesSpy).toHaveBeenLastCalledWith( '', 10, @@ -199,7 +193,6 @@ describe('PipelineList', () => { undefined, undefined, ); ->>>>>>> bbe4ad0a6 (format code with prettifier) expect(updateBannerSpy).toHaveBeenLastCalledWith({}); }); @@ -225,9 +218,6 @@ describe('PipelineList', () => { TestUtils.makeErrorResponseOnce(listPipelinesSpy, 'bad stuff happened'); await refreshBtn!.action(); expect(listPipelinesSpy.mock.calls.length).toBe(2); -<<<<<<< HEAD - expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc', ''); -======= expect(listPipelinesSpy).toHaveBeenLastCalledWith( '', 10, @@ -236,7 +226,6 @@ describe('PipelineList', () => { undefined, undefined, ); ->>>>>>> bbe4ad0a6 (format code with prettifier) expect(updateBannerSpy).toHaveBeenLastCalledWith( expect.objectContaining({ additionalInfo: 'bad stuff happened', @@ -612,8 +601,6 @@ describe('PipelineList', () => { open: true, }); }); -<<<<<<< HEAD -======= it("doesn't keep error message for request from previous namespace", async () => { listPipelinesSpy.mockImplementation(() => Promise.reject('namespace cannot be empty')); @@ -638,5 +625,4 @@ describe('PipelineList', () => { {}, // Empty object means banner has no error message ); }); ->>>>>>> bbe4ad0a6 (format code with prettifier) }); diff --git a/frontend/src/pages/PipelineList.tsx b/frontend/src/pages/PipelineList.tsx index 07735f867a8..2cea54d756b 100644 --- a/frontend/src/pages/PipelineList.tsx +++ b/frontend/src/pages/PipelineList.tsx @@ -36,6 +36,7 @@ import Buttons, { ButtonKeys } from '../lib/Buttons'; import { errorToMessage, formatDateString } from '../lib/Utils'; import { Page } from './Page'; import PipelineVersionList from './PipelineVersionList'; +import { NamespaceContext } from 'src/lib/KubeflowClient'; interface DisplayPipeline extends ApiPipeline { expandState?: ExpandState; @@ -57,7 +58,7 @@ const descriptionCustomRenderer: React.FC> = ( return ; }; -class PipelineList extends Page<{}, PipelineListState> { +export class PipelineList extends Page<{ namespace?: string }, PipelineListState> { private _tableRef = React.createRef(); constructor(props: any) { @@ -176,6 +177,8 @@ class PipelineList extends Page<{}, PipelineListState> { request.pageSize, request.sortBy, request.filter, + this.props.namespace ? 'NAMESPACE' : undefined, + this.props.namespace || undefined, ); displayPipelines = response.pipelines || []; displayPipelines.forEach(exp => (exp.expandState = ExpandState.COLLAPSED)); @@ -247,10 +250,6 @@ class PipelineList extends Page<{}, PipelineListState> { try { method === ImportMethod.LOCAL -<<<<<<< HEAD - ? await Apis.uploadPipeline(name, description || '', file!) - : await Apis.pipelineServiceApi.createPipeline({ name, url: { pipeline_url: url } }); -======= ? await Apis.uploadPipeline(name, description || '', file!, this.props.namespace) : await Apis.pipelineServiceApi.createPipeline({ name, @@ -268,7 +267,6 @@ class PipelineList extends Page<{}, PipelineListState> { : '', }); ->>>>>>> bbe4ad0a6 (format code with prettifier) this.setStateSafe({ uploadDialogOpen: false }); this.refresh(); return true; @@ -284,4 +282,9 @@ class PipelineList extends Page<{}, PipelineListState> { } } -export default PipelineList; +const EnhancedPipelineList: React.FC = props => { + const namespace = React.useContext(NamespaceContext); + return ; +}; + +export default EnhancedPipelineList; From 70aee9e15751bdb7b553b75aad5b12d8533f6e27 Mon Sep 17 00:00:00 2001 From: grobbie Date: Wed, 23 Mar 2022 08:30:48 +0100 Subject: [PATCH 4/6] uploadPipelineVersion method missing optional namespace parameter --- frontend/src/lib/Apis.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/src/lib/Apis.ts b/frontend/src/lib/Apis.ts index dceb1f24b66..599558ff37f 100644 --- a/frontend/src/lib/Apis.ts +++ b/frontend/src/lib/Apis.ts @@ -346,6 +346,7 @@ export class Apis { pipelineId: string, versionData: File, description?: string, + namespace?: string, ): Promise { const fd = new FormData(); fd.append('uploadfile', versionData, versionData.name); From bbabbcc66f65cb403ac815c0b8413db353865691 Mon Sep 17 00:00:00 2001 From: grobbie Date: Wed, 23 Mar 2022 09:10:25 +0100 Subject: [PATCH 5/6] uploadPipeline method missing optional namespace parameter --- frontend/src/lib/Apis.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/src/lib/Apis.ts b/frontend/src/lib/Apis.ts index 599558ff37f..d27dc56648b 100644 --- a/frontend/src/lib/Apis.ts +++ b/frontend/src/lib/Apis.ts @@ -324,6 +324,7 @@ export class Apis { pipelineName: string, pipelineDescription: string, pipelineData: File, + namespace?: string, ): Promise { const fd = new FormData(); fd.append('uploadfile', pipelineData, pipelineData.name); @@ -332,7 +333,7 @@ export class Apis { v1beta1Prefix, `name=${encodeURIComponent(pipelineName)}&description=${encodeURIComponent( pipelineDescription, - )}`, + )}` + (namespace ? `&namespace=${encodeURIComponent(namespace)}` : ''), { body: fd, cache: 'no-cache', From 68286539435232485ea06631b9bec834ec8917c5 Mon Sep 17 00:00:00 2001 From: grobbie Date: Wed, 23 Mar 2022 09:28:37 +0100 Subject: [PATCH 6/6] fix various rebasing induced issues --- frontend/src/pages/PipelineList.tsx | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/frontend/src/pages/PipelineList.tsx b/frontend/src/pages/PipelineList.tsx index 2cea54d756b..1f18aaa4693 100644 --- a/frontend/src/pages/PipelineList.tsx +++ b/frontend/src/pages/PipelineList.tsx @@ -19,7 +19,12 @@ import produce from 'immer'; import * as React from 'react'; import { Link } from 'react-router-dom'; import { classes } from 'typestyle'; -import { ApiListPipelinesResponse, ApiPipeline } from '../apis/pipeline'; +import { + ApiListPipelinesResponse, + ApiPipeline, + ApiResourceType, + ApiRelationship, +} from '../apis/pipeline'; import CustomTable, { Column, CustomRendererProps, @@ -34,7 +39,7 @@ import { commonCss, padding } from '../Css'; import { Apis, ListRequest, PipelineSortKeys } from '../lib/Apis'; import Buttons, { ButtonKeys } from '../lib/Buttons'; import { errorToMessage, formatDateString } from '../lib/Utils'; -import { Page } from './Page'; +import { Page, PageProps } from './Page'; import PipelineVersionList from './PipelineVersionList'; import { NamespaceContext } from 'src/lib/KubeflowClient'; @@ -264,7 +269,7 @@ export class PipelineList extends Page<{ namespace?: string }, PipelineListState relationship: ApiRelationship.OWNER, }, ] - : '', + : [], }); this.setStateSafe({ uploadDialogOpen: false });