Bug 2021205: fix git url change validation #10769

abhinandan13jan · 2022-01-04T13:07:28Z

Addresses

https://issues.redhat.com/browse/OCPBUGSM-36949
https://bugzilla.redhat.com/show_bug.cgi

Issue

Reads gitType instead of detectedGitType

Screenshots

Tests

no change

Browser conformance

Chrome / Firefox

openshift-ci · 2022-01-04T13:07:57Z

@abhinandan13jan: This pull request references Bugzilla bug 2021205, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2021205: fix git url change validation

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.

abhinandan13jan · 2022-01-04T13:08:03Z

/kind bug

abhinandan13jan · 2022-01-04T13:09:03Z

/bugzilla refresh

openshift-ci · 2022-01-04T13:09:10Z

@abhinandan13jan: This pull request references Bugzilla bug 2021205, 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.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

/bugzilla refresh

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 · 2022-01-04T14:01:37Z

When the user fixes the incorrect url, the Git type should not be shown. We need to reset the showGitType variable when a valid git type is detected

openshift-ci · 2022-01-04T15:59:42Z

@abhinandan13jan: This pull request references Bugzilla bug 2021205, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2021205: fix git url change validation

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.

abhinandan13jan · 2022-01-04T16:00:53Z

When the user fixes the incorrect url, the Git type should not be shown. We need to reset the showGitType variable when a valid git type is detected

Fixed

karthikjeeyar · 2022-01-05T05:48:39Z

/lgtm

abhinandan13jan · 2022-01-05T09:43:14Z

/test e2e-gcp-console

christoph-jerolimov · 2022-01-05T10:54:28Z

/assign

christoph-jerolimov

In the old version, the user must select the Git provider if the URL detection doesn't work.

This single case is fixed in the new version, but it breaks the option to select a provider .. and maybe switch it after that. Checkout this recoding:

/hold

openshift-ci · 2022-01-06T08:55:18Z

@abhinandan13jan: This pull request references Bugzilla bug 2021205, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2021205: fix git url change validation

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.

christoph-jerolimov · 2022-01-06T09:17:20Z

frontend/packages/dev-console/src/components/import/git/GitSection.tsx

      }
+
+      if (
+        detectedGitType === GitTypes.github ||
+        detectedGitType === GitTypes.gitlab ||
+        detectedGitType === GitTypes.bitbucket
+      ) {


Does this also if we compare it to "not unsure" (aka sure) instead of list all (currently) known Git provider?

You can optional add the && showGitTypes so that setFieldValue is not called if it's already false.

Suggested change

}

if (

detectedGitType === GitTypes.github ||

detectedGitType === GitTypes.gitlab ||

detectedGitType === GitTypes.bitbucket

) {

} else if (detectedGitType !== GitTypes.unsure && values.git.showGitType) {

i am now using a cleaner approach to the problem. Updated

christoph-jerolimov · 2022-01-06T09:19:20Z

frontend/packages/dev-console/src/components/import/git/GitSection.tsx

-        gitType as any,
+        detectedGitType as any,


~~I'm not sure, but is this change really needed? Doesn't it make sense to use the selected type if we show this dropdown option?~~

Sorry, but I'm now even more sure that this is not what we want here. If the user selects a git provider for a custom domain we should his/her selection instead of the latest detection.

For example when the user enters https://git.mycomapny.com and selects GitLab and changes the repo name again we should still use GitLab to detect the import strategies.

abhinandan13jan · 2022-01-06T09:20:41Z

In the old version, the user must select the Git provider if the URL detection doesn't work.

This single case is fixed in the new version, but it breaks the option to select a provider .. and maybe switch it after that. Checkout this recoding:

/hold

I updated

christoph-jerolimov · 2022-01-06T09:21:16Z

Works better. I unhold this if someone else will take a look. I added two questions and will take a deeper look later.

/unhold

openshift-ci · 2022-01-06T10:24:06Z

@abhinandan13jan: This pull request references Bugzilla bug 2021205, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2021205: fix git url change validation

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.

abhinandan13jan · 2022-01-06T13:17:56Z

/test e2e-gcp-console

karthikjeeyar · 2022-01-07T08:19:59Z

/lgtm

christoph-jerolimov · 2022-01-08T19:58:17Z

/retest

christoph-jerolimov

Hi @abhinandan13jan, cc @karthikjeeyar,

the reason my review takes so long was that I'm not happy that handleGitUrlChange was now called twice when switching from an invalid to a valid git URL.

This happen because const handleGitUrlChange = React.useCallback(... updates the git type in line 252. The useEffect in line 421 then calls the same handleGitUrlChange again with a debounce.

I think we could fix it by cleaning the touched info when we automatically override the git.type in line 251. See suggestion below.

Did you see any reasons, not to do this? IMHO this works only because the useEffect has a (for me not really understandable) condition for different "touched" fields. Btw: I'm not a fan of this solution and that the InputField onChange, this callback, and the effect works so closely together, but that is something we should maybe cleanup in the future.

christoph-jerolimov · 2022-01-08T20:23:09Z

frontend/packages/dev-console/src/components/import/git/GitSection.tsx

      if (gitType !== values.git.type) {
        setFieldValue('git.type', gitType);
      }


Suggested change

if (gitType !== values.git.type) {

setFieldValue('git.type', gitType);

}

if (gitType !== values.git.type) {

setFieldTouched('git.type', false, false);

setFieldValue('git.type', gitType, true);

}

With this change:

Thanks for the feedback, I updated accordingly :)

christoph-jerolimov · 2022-01-08T20:32:01Z

@abhinandan13jan @karthikjeeyar I replace Karthiks lgtm with my approval so that you can update this on Monday without my additional review. But I will do it if needed.

I hope that's fine for you both. If you both agree that we can not apply my recommendation, Karthik can just re-add his lgtm. 😄

/lgtm cancel
/approve

karthikjeeyar · 2022-01-10T06:50:48Z

@abhinandan13jan I agree with Christoph's concern, we should try avoid multiple calls for git detection. Please update the PR.

Thanks @jerolimov for the review and highlighting the issue with multiple network calls, which I overlooked. However I feel as reviewers we should not give lgtm / approve unless we are satisfied with the changes.

karthikjeeyar · 2022-01-10T13:28:04Z

/lgtm

openshift-ci · 2022-01-10T13:28:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, jerolimov, 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/dev-console/OWNERS~~ [jerolimov]

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

openshift-bot · 2022-01-10T14:51:07Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-10T17:19:06Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-10T18:50:10Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-01-10T19:21:30Z

@abhinandan13jan: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci · 2022-01-10T19:23:13Z

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

openshift/console#10769

Bugzilla bug 2021205 has been moved to the MODIFIED state.

In response to this:

Bug 2021205: fix git url change validation

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.

abhinandan13jan changed the title ~~fix git url change validation~~ Bug 2021205: fix git url change validation Jan 4, 2022

openshift-ci bot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 4, 2022

openshift-ci bot requested review from karthikjeeyar and vikram-raj January 4, 2022 13:07

openshift-ci bot added the component/dev-console Related to dev-console label Jan 4, 2022

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 4, 2022

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 4, 2022

openshift-ci bot requested a review from sanketpathak January 4, 2022 13:09

abhinandan13jan force-pushed the fix-git-type-detectection branch from 6eee15a to 2235291 Compare January 4, 2022 15:58

openshift-ci bot assigned karthikjeeyar Jan 5, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2022

openshift-ci bot assigned christoph-jerolimov Jan 5, 2022

christoph-jerolimov suggested changes Jan 5, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2022

abhinandan13jan force-pushed the fix-git-type-detectection branch from 2235291 to a67a22c Compare January 6, 2022 08:52

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2022

christoph-jerolimov reviewed Jan 6, 2022

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2022

abhinandan13jan force-pushed the fix-git-type-detectection branch from a67a22c to c619932 Compare January 6, 2022 10:41

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2022

christoph-jerolimov reviewed Jan 8, 2022

View reviewed changes

openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 8, 2022

fix git url change validation

c0eb4c5

abhinandan13jan force-pushed the fix-git-type-detectection branch from c619932 to c0eb4c5 Compare January 10, 2022 13:06

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2022

openshift-merge-robot merged commit 5e07d72 into openshift:master Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2021205: fix git url change validation #10769

Bug 2021205: fix git url change validation #10769

abhinandan13jan commented Jan 4, 2022 •

edited

Loading

openshift-ci bot commented Jan 4, 2022

abhinandan13jan commented Jan 4, 2022

abhinandan13jan commented Jan 4, 2022

openshift-ci bot commented Jan 4, 2022

karthikjeeyar commented Jan 4, 2022

openshift-ci bot commented Jan 4, 2022

abhinandan13jan commented Jan 4, 2022

karthikjeeyar commented Jan 5, 2022

abhinandan13jan commented Jan 5, 2022

christoph-jerolimov commented Jan 5, 2022

christoph-jerolimov left a comment

openshift-ci bot commented Jan 6, 2022

christoph-jerolimov Jan 6, 2022

abhinandan13jan Jan 6, 2022

christoph-jerolimov Jan 6, 2022 •

edited

Loading

abhinandan13jan Jan 6, 2022

abhinandan13jan commented Jan 6, 2022

christoph-jerolimov commented Jan 6, 2022 •

edited

Loading

openshift-ci bot commented Jan 6, 2022

abhinandan13jan commented Jan 6, 2022

karthikjeeyar commented Jan 7, 2022

christoph-jerolimov commented Jan 8, 2022

christoph-jerolimov left a comment

christoph-jerolimov Jan 8, 2022

abhinandan13jan Jan 10, 2022

christoph-jerolimov commented Jan 8, 2022 •

edited

Loading

karthikjeeyar commented Jan 10, 2022

karthikjeeyar commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

openshift-bot commented Jan 10, 2022

openshift-bot commented Jan 10, 2022

openshift-bot commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

Bug 2021205: fix git url change validation #10769

Bug 2021205: fix git url change validation #10769

Conversation

abhinandan13jan commented Jan 4, 2022 • edited Loading

Addresses

Issue

Screenshots

Tests

Browser conformance

openshift-ci bot commented Jan 4, 2022

abhinandan13jan commented Jan 4, 2022

abhinandan13jan commented Jan 4, 2022

openshift-ci bot commented Jan 4, 2022

karthikjeeyar commented Jan 4, 2022

openshift-ci bot commented Jan 4, 2022

abhinandan13jan commented Jan 4, 2022

karthikjeeyar commented Jan 5, 2022

abhinandan13jan commented Jan 5, 2022

christoph-jerolimov commented Jan 5, 2022

christoph-jerolimov left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 6, 2022

christoph-jerolimov Jan 6, 2022

Choose a reason for hiding this comment

abhinandan13jan Jan 6, 2022

Choose a reason for hiding this comment

christoph-jerolimov Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

abhinandan13jan Jan 6, 2022

Choose a reason for hiding this comment

abhinandan13jan commented Jan 6, 2022

christoph-jerolimov commented Jan 6, 2022 • edited Loading

openshift-ci bot commented Jan 6, 2022

abhinandan13jan commented Jan 6, 2022

karthikjeeyar commented Jan 7, 2022

christoph-jerolimov commented Jan 8, 2022

christoph-jerolimov left a comment

Choose a reason for hiding this comment

christoph-jerolimov Jan 8, 2022

Choose a reason for hiding this comment

abhinandan13jan Jan 10, 2022

Choose a reason for hiding this comment

christoph-jerolimov commented Jan 8, 2022 • edited Loading

karthikjeeyar commented Jan 10, 2022

karthikjeeyar commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

openshift-bot commented Jan 10, 2022

openshift-bot commented Jan 10, 2022

openshift-bot commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

abhinandan13jan commented Jan 4, 2022 •

edited

Loading

christoph-jerolimov Jan 6, 2022 •

edited

Loading

christoph-jerolimov commented Jan 6, 2022 •

edited

Loading

christoph-jerolimov commented Jan 8, 2022 •

edited

Loading