implementing bundle feature #694

sudhirverma · 2022-10-04T07:16:33Z

codecov-commenter · 2022-10-04T07:20:51Z

Codecov Report

Base: 67.92% // Head: 67.17% // Decreases project coverage by -0.75% ⚠️

Coverage data is based on head (00ddcd1) compared to base (ee598cb).
Patch coverage: 40.22% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #694      +/-   ##
==========================================
- Coverage   67.92%   67.17%   -0.76%     
==========================================
  Files         114      117       +3     
  Lines        6619     6790     +171     
  Branches     1157     1179      +22     
==========================================
+ Hits         4496     4561      +65     
- Misses       2123     2229     +106

Impacted Files	Coverage Δ
src/cluster-version.ts	`53.84% <ø> (ø)`
src/pipeline/preview.ts	`19.53% <ø> (ø)`
src/pipeline/wizard.ts	`68.81% <ø> (ø)`
src/tekton/restartpipelinerundata.ts	`39.06% <ø> (ø)`
src/tkn.ts	`77.97% <ø> (ø)`
src/yaml-support/tkn-code-actions.ts	`93.45% <ø> (ø)`
src/yaml-support/tkn-definition-providers.ts	`96.66% <ø> (ø)`
src/yaml-support/tkn-yaml-scheme-generator.ts	`93.54% <ø> (ø)`
src/pipeline/bundle.ts	`18.98% <18.98%> (ø)`
src/util/watchResources.ts	`72.97% <33.33%> (-0.64%)`	⬇️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mohitsuman · 2022-10-15T14:02:44Z

@sudhirverma which issue this PR is related to?

sudhirverma · 2022-10-15T15:35:00Z

@sudhirverma which issue this PR is related to?

this PR will fix #695

mohitsuman · 2022-10-15T16:14:37Z

@sudhirverma is the PR ready for review or you need to add some more features to it ?

sudhirverma · 2022-10-15T16:36:01Z

@sudhirverma is the PR ready for review or you need to add some more features to it ?

I am still working on it. I will add you as a reviewer when this PR is ready.

sudhirverma · 2022-10-31T18:10:18Z

Step to test this PR, enable enable-tekton-oci-bundles is set to "true" in the feature-flags configmap in tekton-pipelines namespace.

Screen.Recording.2022-10-31.at.11.33.20.PM.mov

mohitsuman · 2022-11-01T09:59:47Z

@sudhirverma Provide the registry username/password also in the same form. User need not go to switch the workflow. All Qs should be in the same form and once user clicks Submit, it should perform the required step.

lstocchi

Started playing with it.

I faced this error when clicking on submit.

The other annoying part is that the webview closes and when i reopen it the form is blank so i need to re-fill it to re-try deploying it. An idea could be to close the webview only if the deploy is successful
If i click on the bundle action multiple time the webview gets opened in multiple tabs. Wouldn't it be enough to only have one webview opened? I don't think anybody is going to create 2 bundles at the same time.
(person taste) isn't the webview too white compared to the vscode theme? The first time I opened it i got blind 😆
The autocomplete item (= tekton resources) has one subtle issue. If you have two resources of different type with the same name you cannot add both of them.
It would be good if, once you choose a resource in the autocomplete list, this gets removed from the list.
We should enable the submit button only if the image name has a valid format

sudhirverma · 2022-11-09T11:37:43Z

Started playing with it.

I faced this error when clicking on submit.

The other annoying part is that the webview closes and when i reopen it the form is blank so i need to re-fill it to re-try deploying it. An idea could be to close the webview only if the deploy is successful

If i click on the bundle action multiple time the webview gets opened in multiple tabs. Wouldn't it be enough to only have one webview opened? I don't think anybody is going to create 2 bundles at the same time.

(person taste) isn't the webview too white compared to the vscode theme? The first time I opened it i got blind 😆

The autocomplete item (= tekton resources) has one subtle issue. If you have two resources of different type with the same name you cannot add both of them.

It would be good if, once you choose a resource in the autocomplete list, this gets removed from the list.

We should enable the submit button only if the image name has a valid format

done

lstocchi

LGTM
I assume this PR is just for creating a new bundle and you're going to add the feature to download a bundle and allow users to import one or more resources contained in it in a new PR, right?

As a future enhancement, if it's possible to add multiple selection for the tekton resource dropdownlist would be great. When adding 10 resources it's an annoying process otherwise

sudhirverma · 2022-11-09T17:56:22Z

I assume this PR is just for creating a new bundle and you're going to add the feature to download a bundle and allow users to import one or more resources contained in it in a new PR, right?

Yes I will create a new PR.

As a future enhancement, if it's possible to add multiple selection for the tekton resource dropdownlist would be great. When adding 10 resources it's an annoying process otherwise

Multiple selection is there when you press Ctrl and click on resources I will check if we can select multiple resource without pressing the ctrl key.

mohitsuman · 2022-11-11T11:30:13Z

@sudhirverma for multi-select chip selection, this should work https://mui.com/material-ui/react-select/#chip. This should be quick to do.

sudhirverma · 2022-11-11T19:03:22Z

@sudhirverma for multi-select chip selection, this should work https://mui.com/material-ui/react-select/#chip. This should be quick to do.

done

mohitsuman · 2022-11-15T21:50:08Z

@sudhirverma please add a gif of how the select chips look after the changes are done. And also how the submit button looks like.

sudhirverma · 2022-11-16T17:54:24Z

@mohitsuman attaching video

2022-11-16.23-20-50.mp4

mohitsuman · 2022-11-17T11:04:54Z

@sudhirverma There are multiple CSS stuff which needs to be improved here.

The submit button text colour.
The input boxes width (we need to keep it small, not so big)
Tekton resources layout breaks, as in overlaps with the Tekton Resources field name(at 0:22 in the video shared)

For input box layout, refer to how we do in the OpenShift extension https://github.com/redhat-developer/vscode-openshift-tools/blob/main/src/webview/git-import/app/gitImport.tsx#L322 . This will help you to improve the CSS also.

sudhirverma · 2022-11-18T16:04:45Z

@mohitsuman Updated the PR adding gif also

sudhirverma · 2022-11-24T09:59:05Z

@mohitsuman done can you check

mohitsuman · 2022-11-25T20:00:31Z

@sudhirverma This PR fails if we try to run this on a connected OpenShift cluster. It asks for namespaces "tekton-pipelines". For OpenShift it should be the openshift-pipelines namespace. And please also mention the detail step to configure the features to true to run bundles.

mohitsuman · 2022-11-25T20:07:03Z

Ideally this is the place to update the feature flag: $CLUSTER_URL/k8s/ns/openshift-pipelines/configmaps/feature-flags/yaml

mohitsuman

Multiple changes needs to be fixed. The most important one is support openshift-pipelines namespace.

mohitsuman · 2022-11-25T21:12:48Z

src/webview/bundle/Form.tsx

+        <Typography variant="h5" > Create Bundle</Typography>
+      </div>
+      <div className='subTitle'>
+        <Typography>This workflow will help to create a bundle and push it to remote registry.</Typography>


Suggested change

<Typography>This workflow will help to create a bundle and push it to remote registry.</Typography>

<Typography>This workflow will help to create your own Tekton bundle, push it to the remote registry and reference that in your Tekton manifest. Before running this command make sure you are authenticated to the remote image registry and valid credentials are there in order to push to that registry.</Typography>

done I have changed the subtitle

mohitsuman · 2022-11-25T21:14:27Z

src/webview/bundle/Form.tsx

+  return (
+    <div className='mainContainer margin' >
+      <div className='title'>
+        <Typography variant="h5" > Create Bundle</Typography>


Suggested change

<Typography variant="h5" > Create Bundle</Typography>

<Typography variant="h5" >Publish Tekton Resources as bundles on OCI registry</Typography>

done fix I have fixed the title

mohitsuman · 2022-11-25T21:17:46Z

src/pipeline/bundle.ts

+      BundleWizard.currentPanel.editor.reveal(column);
+      return;
+    }
+    const bundleTitle = 'Create Bundle';


Suggested change

const bundleTitle = 'Create Bundle';

const bundleTitle = 'Create Tekton Bundle';

done the changes.

mohitsuman · 2022-11-25T21:18:42Z

src/tekton/bundle-pipeline-task.ts

+  const featureFlagData: FeatureFlag = await checkEnableApiFields();
+  if (!featureFlagData) return null;
+  if (featureFlagData.data['enable-tekton-oci-bundles'] !== 'true') {
+    window.showWarningMessage('To enable bundles change enable-tekton-oci-bundles to "true" in "feature-flags" ConfigMap namespace tekton-pipelines');


tekton-pipelines should not be hardcoded. This can be for openshift-pipelines also.

I have removed the hardcoded. Now it will show warning message as per cluster.

mohitsuman · 2022-11-25T21:19:46Z

src/tekton/bundle-pipeline-task.ts

+  return await window.withProgress({title: 'Bundling image.....', location: ProgressLocation.Notification}, async () => {
+    const result = await tkn.execute(Command.bundle(bundleInfo.imageDetail, `${quote}${fsPath}${quote}`, bundleInfo.userDetail, bundleInfo.passwordDetail), process.cwd(), false);
+    if (result.error) {
+      window.showErrorMessage(`Failed to push bundle error: ${getStderrString(result.error)}`);


Suggested change

window.showErrorMessage(`Failed to push bundle error: ${getStderrString(result.error)}`);

window.showErrorMessage(`Failed to push the bundle. Please check the following error: ${getStderrString(result.error)}`);

I have changed the error message as per your suggestion.

mohitsuman · 2022-11-25T21:28:23Z

src/webview/bundle/index.tsx

+const previousState = vscode.getState();
+if (previousState) {
+  ReactDOM.render(
+    <React.StrictMode>


Same here, the App should work without StrictMode.

I have removed StrictMode

mohitsuman · 2022-11-25T21:29:21Z

src/webview/bundle/style.scss

+
+.subTitle {
+    margin-bottom: 2rem;
+    max-width: 59rem;


59 rem is too much of a max-width. It needs to be a percentage.

I am using percentage now

mohitsuman · 2022-11-25T21:29:47Z

src/webview/bundle/style.scss

+    margin-bottom: 2rem;
+    max-width: 59rem;
+    justify-content: center;
+    word-spacing: 5px;


Either use rem or px. Do not use both.

I am using percentage now

mohitsuman · 2022-11-25T21:31:30Z

test/coverage.ts

@@ -27,126 +27,126 @@ export interface TestRunnerOptions {

 export class CoverageRunner {



@sudhirverma Coverage fix should be in a different PR. It does not relate to this PR fix.

It's needed to pass the build

mohitsuman · 2022-11-25T21:32:54Z

package.json

@@ -1056,6 +1072,9 @@
    "ui-test": "extest setup-and-run ./out/ui-test/all.js"
  },
  "devDependencies": {
+    "@babel/core": "7.19.6",
+    "@emotion/react": "^11.7.1",


Why is this dependency needed ?

mohitsuman · 2022-11-29T19:38:56Z

@sudhirverma looks good to me. Just resolve the conflicts and we should be good to merge.

@lstocchi can you please look at the bundle logic and approve?

…nly one time in editor

mohitsuman

LGTM

sudhirverma force-pushed the bundle branch 2 times, most recently from 8e8e437 to bd378df Compare October 31, 2022 12:35

mohitsuman requested a review from lstocchi November 1, 2022 12:01

lstocchi requested changes Nov 2, 2022

View reviewed changes

odockal mentioned this pull request Nov 8, 2022

vsce package command fails with latest react components in version 18 #699

Closed

sudhirverma requested a review from lstocchi November 8, 2022 16:40

lstocchi approved these changes Nov 9, 2022

View reviewed changes

sudhirverma force-pushed the bundle branch from 82fc64d to 90aa452 Compare November 17, 2022 10:57

sudhirverma force-pushed the bundle branch from a517485 to 23219fe Compare November 24, 2022 06:35

mohitsuman requested changes Nov 25, 2022

View reviewed changes

mohitsuman self-requested a review November 25, 2022 21:39

sudhirverma added 25 commits November 30, 2022 16:15

Fix eslint error

427b153

Fix password input field

3a1cee3

Fixing css

ddffb2c

Fixing css

4bb9f8f

Fix button css

17b863d

Fix css for input

e68bf2e

Fix css for UI

d719387

fix css label in Chip

592b5a1

working on username & password and fixing bundle action should open o…

979d69b

…nly one time in editor

bundle action to open single time in editor

737e6dd

fix validation for input image and password and username

7ce2800

Fix build

1a906da

Fix review changes

fb2f88c

Fix dropdown seletct issue

8e48761

Rebase and fix UI

5f76754

Fix UI css

fe7d200

Fix css

cb9488d

Fixing build

6a646c5

Fixing build

3c4faa8

Fix css Autocomplete

bd550da

Fix review changes

cc30314

Fix build

5342c91

Fix unit test

9736843

Remove check for enable-tekton-oci-bundles

ce6c5e9

rebase

dd23655

sudhirverma force-pushed the bundle branch from ef324a3 to dd23655 Compare November 30, 2022 11:20

sudhirverma added 2 commits November 30, 2022 18:46

Fix build

e763769

Fix indentation

5576f8d

mohitsuman approved these changes Nov 30, 2022

View reviewed changes

mohitsuman merged commit 1b1c885 into redhat-developer:main Nov 30, 2022

	<Typography>This workflow will help to create a bundle and push it to remote registry.</Typography>
	<Typography>This workflow will help to create your own Tekton bundle, push it to the remote registry and reference that in your Tekton manifest. Before running this command make sure you are authenticated to the remote image registry and valid credentials are there in order to push to that registry.</Typography>

	<Typography variant="h5" > Create Bundle</Typography>
	<Typography variant="h5" >Publish Tekton Resources as bundles on OCI registry</Typography>

	const bundleTitle = 'Create Bundle';
	const bundleTitle = 'Create Tekton Bundle';

	window.showErrorMessage(`Failed to push bundle error: ${getStderrString(result.error)}`);
	window.showErrorMessage(`Failed to push the bundle. Please check the following error: ${getStderrString(result.error)}`);

		@@ -27,126 +27,126 @@ export interface TestRunnerOptions {

		export class CoverageRunner {

implementing bundle feature #694

implementing bundle feature #694

Conversation

sudhirverma commented Oct 4, 2022 • edited Loading

codecov-commenter commented Oct 4, 2022 • edited Loading

Codecov Report

mohitsuman commented Oct 15, 2022

sudhirverma commented Oct 15, 2022

mohitsuman commented Oct 15, 2022

sudhirverma commented Oct 15, 2022

sudhirverma commented Oct 31, 2022

mohitsuman commented Nov 1, 2022

lstocchi left a comment

Choose a reason for hiding this comment

sudhirverma commented Nov 9, 2022

lstocchi left a comment

Choose a reason for hiding this comment

sudhirverma commented Nov 9, 2022

mohitsuman commented Nov 11, 2022

sudhirverma commented Nov 11, 2022

mohitsuman commented Nov 15, 2022

sudhirverma commented Nov 16, 2022

mohitsuman commented Nov 17, 2022

sudhirverma commented Nov 18, 2022

sudhirverma commented Nov 24, 2022

mohitsuman commented Nov 25, 2022

mohitsuman commented Nov 25, 2022

mohitsuman left a 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

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

sudhirverma Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohitsuman commented Nov 29, 2022

mohitsuman left a comment

Choose a reason for hiding this comment

sudhirverma commented Oct 4, 2022 •

edited

Loading

codecov-commenter commented Oct 4, 2022 •

edited

Loading

sudhirverma Nov 28, 2022 •

edited

Loading