Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource #8051

invincibleJai · 2021-02-04T08:16:28Z

Fixes:
https://issues.redhat.com/browse/ODC-5458

Analysis / Root cause:
In form view for kafksSource if sasl/tls are enebled then other fields i.e user , password or caCert, cert, key where shown as required field which shouldn't be the case

Solution Description:
Removed required validation from net section and rely on API validation for same

Screen shots / Gifs for design review:

Test:

Test setup:

Install Serverless operator
create CR for Knative Kafka (enable source) along with serving and eventing

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2021-02-04T08:33:06Z

@invincibleJai: This pull request references Bugzilla bug 1925024, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

karthikjeeyar · 2021-02-04T10:26:35Z

We could add a test spec against the net section

    it('should not throw error if net section is empty', async () => {
      const defaultEventingData = getDefaultEventingData(EventSources.KafkaSource);
      const mockData = _.cloneDeep(defaultEventingData);
      delete mockData.formData.data.KafkaSource.net;
      await eventSourceValidationSchema(t)
        .resolve({ value: mockData })
        .isValid(mockData)
        .then((valid) => expect(valid).toEqual(true));
    });

openshift-ci-robot · 2021-02-04T12:08:22Z

@invincibleJai: This pull request references Bugzilla bug 1925024, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

karthikjeeyar · 2021-02-04T12:58:31Z

frontend/packages/knative-plugin/src/utils/__tests__/create-eventsources-utils.spec.ts

+    expect(
+      isSecretKeyRefPresent({ secretKeyRef: { name: 'my-sasl-secret', key: '' } }),
+    ).toBeTruthy();
+    expect(isSecretKeyRefPresent({ secretKeyRef: { name: '', key: 'user' } })).toBeTruthy();


When asserting against a exact boolean value we should go for .toBe(true) or .toBe(false).

see this - https://jestjs.io/docs/en/expect#tobetruthy

In JavaScript, there are six falsy values: false, 0, '', null, undefined, and NaN. Everything else is truthy.

karthikjeeyar · 2021-02-04T12:58:39Z

frontend/packages/knative-plugin/src/utils/__tests__/create-eventsources-utils.spec.ts

+  });
+
+  it('expect false if secretKeyRef is not there for name and key', () => {
+    expect(isSecretKeyRefPresent({ secretKeyRef: { name: '', key: '' } })).toBeFalsy();


karthikjeeyar · 2021-02-04T13:06:33Z

frontend/packages/knative-plugin/src/utils/__tests__/create-eventsources-utils.spec.ts

@@ -297,4 +299,83 @@ describe('sanitizeSourceToForm always returns valid Event Source', () => {
    expect(formData.application.name).toEqual('');
    expect(formData.application.selectedKey).toEqual(UNASSIGNED_KEY);
  });
+


Pls Add the null test as well

Suggested change

expect(isSecretKeyRefPresent(null)).toBe(false);

karthikjeeyar · 2021-02-04T14:15:39Z

Verified locally, works fine
/lgtm

openshift-ci-robot · 2021-02-04T14:15:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, karthikjeeyar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/knative-plugin/OWNERS~~ [invincibleJai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2021-02-04T22:11:27Z

@invincibleJai: All pull requests linked via external trackers have merged:

openshift/console#8051

Bugzilla bug 1925024 has been moved to the MODIFIED state.

In response to this:

Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

invincibleJai · 2021-02-17T07:27:29Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-02-17T07:28:07Z

@invincibleJai: #8051 failed to apply on top of branch "release-4.6":

Applying: removes form validation from net(sasl/tls) section for kafkaSource
Using index info to reconstruct a base tree...
M	frontend/packages/knative-plugin/src/components/add/__tests__/eventSource-validation-utils.spec.ts
M	frontend/packages/knative-plugin/src/components/add/eventSource-validation-utils.ts
M	frontend/packages/knative-plugin/src/utils/__tests__/create-eventsources-utils.spec.ts
M	frontend/packages/knative-plugin/src/utils/create-eventsources-utils.ts
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/knative-plugin/src/utils/create-eventsources-utils.ts
CONFLICT (content): Merge conflict in frontend/packages/knative-plugin/src/utils/create-eventsources-utils.ts
Auto-merging frontend/packages/knative-plugin/src/utils/__tests__/create-eventsources-utils.spec.ts
CONFLICT (content): Merge conflict in frontend/packages/knative-plugin/src/utils/__tests__/create-eventsources-utils.spec.ts
Auto-merging frontend/packages/knative-plugin/src/components/add/eventSource-validation-utils.ts
CONFLICT (content): Merge conflict in frontend/packages/knative-plugin/src/components/add/eventSource-validation-utils.ts
Auto-merging frontend/packages/knative-plugin/src/components/add/__tests__/eventSource-validation-utils.spec.ts
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 removes form validation from net(sasl/tls) section for kafkaSource
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from abhinandan13jan and divyanshiGupta February 4, 2021 08:16

openshift-ci-robot added component/knative Related to knative-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 4, 2021

invincibleJai changed the title ~~removes form validation from net(sasl/tls) section for kafkaSource~~ Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource Feb 4, 2021

openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Feb 4, 2021

invincibleJai force-pushed the odc-5458 branch from ad0dcd2 to 5e9c5b1 Compare February 4, 2021 12:10

karthikjeeyar reviewed Feb 4, 2021

View reviewed changes

invincibleJai force-pushed the odc-5458 branch 2 times, most recently from aba54f3 to b9867f3 Compare February 4, 2021 13:59

removes form validation from net(sasl/tls) section for kafkaSource

08c4148

invincibleJai force-pushed the odc-5458 branch from b9867f3 to 08c4148 Compare February 4, 2021 14:13

openshift-ci-robot assigned karthikjeeyar Feb 4, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2021

openshift-merge-robot merged commit 00e9537 into openshift:master Feb 4, 2021

spadgett added this to the v4.7 milestone Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource #8051

Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource #8051

invincibleJai commented Feb 4, 2021 •

edited

openshift-ci-robot commented Feb 4, 2021

karthikjeeyar commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

karthikjeeyar Feb 4, 2021

karthikjeeyar Feb 4, 2021

karthikjeeyar Feb 4, 2021

invincibleJai Feb 4, 2021

karthikjeeyar commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

invincibleJai commented Feb 17, 2021

openshift-cherrypick-robot commented Feb 17, 2021

Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource #8051

Bug 1925024: removes form validation from net(sasl/tls) section for kafkaSource #8051

Conversation

invincibleJai commented Feb 4, 2021 • edited

openshift-ci-robot commented Feb 4, 2021

karthikjeeyar commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

karthikjeeyar Feb 4, 2021

Choose a reason for hiding this comment

karthikjeeyar Feb 4, 2021

Choose a reason for hiding this comment

karthikjeeyar Feb 4, 2021

Choose a reason for hiding this comment

invincibleJai Feb 4, 2021

Choose a reason for hiding this comment

karthikjeeyar commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

invincibleJai commented Feb 17, 2021

openshift-cherrypick-robot commented Feb 17, 2021

invincibleJai commented Feb 4, 2021 •

edited