Bug 1883306: Include repo branch in URL for che icon #6764

rottencandy · 2020-09-28T17:25:55Z

Fixes:
https://issues.redhat.com/browse/ODC-4385
#6077
https://bugzilla.redhat.com/show_bug.cgi?id=1883306

Analysis / Root cause:
The URL generated for the Che editor icon in topology does not consider the repository branch.

Solution Description:
Include repository branch when creating the URL, in the format ${gitURL}/tree/${gitBranch}.

Test setup:

Install Eclipse Che operator and create a CheCluster instance.
Create an application using Git import flow, optionally adding a different branch in advanced git options.
Notice the generated URL for che icon in the application's topology node contains the repository branch.

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2020-09-28T17:25:58Z

@rottencandy: This pull request references Bugzilla bug 1883306, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" 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 1883306: Include repo branch in URL for che icon

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.

rottencandy · 2020-09-28T17:26:56Z

/kind bug

rottencandy · 2020-09-29T06:54:01Z

/retest

frontend/packages/dev-console/src/components/topology/topology-utils.ts

jerolimov

Tested this locally with a server which could not start che, but the link looks good.

divyanshiGupta · 2020-09-29T12:59:49Z

@saud I think when we do not have the cheURL then the returned gitURL should also have branchRef if present.

rottencandy · 2020-09-29T13:27:15Z

@saud I think when we do not have the cheURL then the returned gitURL should also have branchRef if present.

+1. Updated.

divyanshiGupta · 2020-09-29T14:06:07Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

-  return gitURL && cheURL ? `${cheURL}/f?url=${gitURL}&policies.create=peruser` : gitURL;
+export const getEditURL = (gitURL: string, gitBranch: string, cheURL: string) => {
+  const fullGitURL = gitBranch ? `${gitURL}/tree/${gitBranch}` : gitURL;
+  return cheURL ? `${cheURL}/f?url=${fullGitURL}&policies.create=peruser` : fullGitURL;


Suggested change

return cheURL ? `${cheURL}/f?url=${fullGitURL}&policies.create=peruser` : fullGitURL;

return gitURL && cheURL ? `${cheURL}/f?url=${fullGitURL}&policies.create=peruser` : fullGitURL;

divyanshiGupta

/lgtm

rottencandy · 2020-09-29T14:44:08Z

/retest

rottencandy · 2020-09-29T15:01:47Z

/retest

divyanshiGupta · 2020-09-29T15:06:43Z

@rottencandy you need to fix resource-utils.spec.ts specs.

divyanshiGupta · 2020-09-29T15:08:26Z

/lgtm cancel

jerolimov · 2020-09-29T21:41:35Z

...kages/dev-console/src/components/topology/data-transforms/__tests__/data-transformer.spec.ts

+    const mockGitURL =
+      mockResources.deploymentConfigs.data[0].metadata.annotations['app.openshift.io/vcs-uri'];
+    const mockGitBranch =
+      mockResources.deploymentConfigs.data[0].metadata.annotations['app.openshift.io/vcs-ref'];


Since you check if the branch is defined or not, can you please add add a second test when there is no branch defined.

I've added the check in the next it() block.

jerolimov · 2020-09-29T21:46:44Z

...kages/dev-console/src/components/topology/data-transforms/__tests__/data-transformer.spec.ts

    const mockGitURL =
      mockResources.deploymentConfigs.data[0].metadata.annotations['app.openshift.io/vcs-uri'];
    const graphData = getTransformedTopologyData(mockResources, ['deploymentConfigs']);
    const { editURL, vcsURI } = graphData.nodes[0].data.data as WorkloadData;
-    const editUrl = editURL || getEditURL(vcsURI, '');
+    const editUrl = editURL || getEditURL(vcsURI, '', '');


Suggested change

const editUrl = editURL || getEditURL(vcsURI, '', '');

const editUrl = getEditURL(vcsURI, '', '');

Please remove this "logic" from the test. Each test case should test a strict behavior and I expect that such a fallback is what we want "to test" here.

jerolimov · 2020-09-30T15:30:22Z

Tested this locally and the link looks fine to me.

@divyanshiGupta do you have something from keeping back adding a lgtm label?

divyanshiGupta · 2020-09-30T16:28:00Z

/lgtm

rottencandy · 2020-09-30T16:38:04Z

/assign @christianvogt

openshift-bot · 2020-10-01T01:00:24Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci-robot · 2020-10-01T01:00:26Z

@openshift-bot: This pull request references Bugzilla bug 1883306, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" 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:

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

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 · 2020-10-01T10:04:53Z

/approve

verified the changes, works as expected.

openshift-ci-robot · 2020-10-01T10:05:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyanshiGupta, invincibleJai, jerolimov, rottencandy

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~~ [invincibleJai]

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

rottencandy · 2020-10-01T10:12:25Z

/bugzilla refresh

openshift-ci-robot · 2020-10-01T10:12:33Z

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

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.

openshift-ci-robot · 2020-10-01T12:24:10Z

@rottencandy: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-console	`80f1982`	link	`/test e2e-gcp-console`

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.

rottencandy · 2020-10-01T12:35:46Z

/retest

openshift-ci-robot · 2020-10-01T12:58:44Z

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

openshift/console#6764

Bugzilla bug 1883306 has been moved to the MODIFIED state.

In response to this:

Bug 1883306: Include repo branch in URL for che icon

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.

rottencandy · 2020-10-01T18:26:04Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-10-01T18:26:18Z

@rottencandy: #6764 failed to apply on top of branch "release-4.5":

Applying: include branch in che icon url
Using index info to reconstruct a base tree...
M	frontend/packages/dev-console/src/components/topology/components/nodes/WorkloadNode.tsx
M	frontend/packages/dev-console/src/components/topology/data-transforms/__tests__/data-transformer.spec.ts
M	frontend/packages/dev-console/src/components/topology/data-transforms/transform-utils.ts
M	frontend/packages/dev-console/src/components/topology/topology-types.ts
M	frontend/packages/dev-console/src/components/topology/topology-utils.ts
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/dev-console/src/components/topology/topology-utils.ts
Auto-merging frontend/packages/dev-console/src/components/topology/topology-types.ts
Auto-merging frontend/packages/dev-console/src/components/topology/data-transforms/transform-utils.ts
Auto-merging frontend/packages/dev-console/src/components/topology/data-transforms/__tests__/data-transformer.spec.ts
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/components/topology/data-transforms/__tests__/data-transformer.spec.ts
Auto-merging frontend/packages/dev-console/src/components/topology/components/nodes/WorkloadNode.tsx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 include branch in che icon url
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.5

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.

jerolimov · 2020-10-02T11:09:26Z

@rottencandy Cherry-picked this PR for 4.5. Happy holiday.

openshift-ci-robot requested review from jeff-phillips-18 and jhadvig September 28, 2020 17:26

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 28, 2020

divyanshiGupta reviewed Sep 29, 2020

View reviewed changes

frontend/packages/dev-console/src/components/topology/topology-utils.ts Outdated Show resolved Hide resolved

rottencandy force-pushed the chebranch branch from 9d4b962 to d356171 Compare September 29, 2020 12:17

jerolimov approved these changes Sep 29, 2020

View reviewed changes

rottencandy force-pushed the chebranch branch from d356171 to 169f79d Compare September 29, 2020 13:26

divyanshiGupta reviewed Sep 29, 2020

View reviewed changes

rottencandy force-pushed the chebranch branch from 169f79d to 1869edb Compare September 29, 2020 14:08

divyanshiGupta approved these changes Sep 29, 2020

View reviewed changes

openshift-ci-robot assigned divyanshiGupta Sep 29, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020

rottencandy force-pushed the chebranch branch from 1869edb to a82b18f Compare September 29, 2020 15:32

jerolimov reviewed Sep 29, 2020

View reviewed changes

include branch in che icon url

80f1982

rottencandy force-pushed the chebranch branch from a82b18f to 80f1982 Compare September 30, 2020 07:20

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2020

openshift-ci-robot assigned christianvogt Sep 30, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2020

openshift-ci-robot 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 Oct 1, 2020

openshift-merge-robot merged commit e4324d2 into openshift:master Oct 1, 2020

This was referenced Oct 2, 2020

[release-4.5] Bug 1884574: Include repo branch in URL for che icon #6814

Closed

[release-4.5] Bug 1884574: Include repo branch in URL for che icon #6815

Merged

spadgett added this to the v4.6 milestone Oct 2, 2020

rottencandy mentioned this pull request Nov 9, 2020

When CodeReady Workspaces is installed on the cluster - the link to load devfile should load the devfile from the appropriate branch #6077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1883306: Include repo branch in URL for che icon #6764

Bug 1883306: Include repo branch in URL for che icon #6764

rottencandy commented Sep 28, 2020

openshift-ci-robot commented Sep 28, 2020

rottencandy commented Sep 28, 2020

rottencandy commented Sep 29, 2020

jerolimov left a comment

divyanshiGupta commented Sep 29, 2020

rottencandy commented Sep 29, 2020

divyanshiGupta Sep 29, 2020

divyanshiGupta left a comment

rottencandy commented Sep 29, 2020

rottencandy commented Sep 29, 2020 via email

divyanshiGupta commented Sep 29, 2020

divyanshiGupta commented Sep 29, 2020

jerolimov Sep 29, 2020

rottencandy Sep 30, 2020

jerolimov Sep 29, 2020

rottencandy Sep 30, 2020

jerolimov commented Sep 30, 2020 •

edited

divyanshiGupta commented Sep 30, 2020

rottencandy commented Sep 30, 2020

openshift-bot commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

invincibleJai commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

rottencandy commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

rottencandy commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

rottencandy commented Oct 1, 2020

openshift-cherrypick-robot commented Oct 1, 2020

jerolimov commented Oct 2, 2020 •

edited

	return cheURL ? `${cheURL}/f?url=${fullGitURL}&policies.create=peruser` : fullGitURL;
	return gitURL && cheURL ? `${cheURL}/f?url=${fullGitURL}&policies.create=peruser` : fullGitURL;

	const editUrl = editURL \|\| getEditURL(vcsURI, '', '');
	const editUrl = getEditURL(vcsURI, '', '');

Bug 1883306: Include repo branch in URL for che icon #6764

Bug 1883306: Include repo branch in URL for che icon #6764

Conversation

rottencandy commented Sep 28, 2020

openshift-ci-robot commented Sep 28, 2020

rottencandy commented Sep 28, 2020

rottencandy commented Sep 29, 2020

jerolimov left a comment

Choose a reason for hiding this comment

divyanshiGupta commented Sep 29, 2020

rottencandy commented Sep 29, 2020

divyanshiGupta Sep 29, 2020

Choose a reason for hiding this comment

divyanshiGupta left a comment

Choose a reason for hiding this comment

rottencandy commented Sep 29, 2020

rottencandy commented Sep 29, 2020 via email

divyanshiGupta commented Sep 29, 2020

divyanshiGupta commented Sep 29, 2020

jerolimov Sep 29, 2020

Choose a reason for hiding this comment

rottencandy Sep 30, 2020

Choose a reason for hiding this comment

jerolimov Sep 29, 2020

Choose a reason for hiding this comment

rottencandy Sep 30, 2020

Choose a reason for hiding this comment

jerolimov commented Sep 30, 2020 • edited

divyanshiGupta commented Sep 30, 2020

rottencandy commented Sep 30, 2020

openshift-bot commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

invincibleJai commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

rottencandy commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

rottencandy commented Oct 1, 2020

openshift-ci-robot commented Oct 1, 2020

rottencandy commented Oct 1, 2020

openshift-cherrypick-robot commented Oct 1, 2020

jerolimov commented Oct 2, 2020 • edited

jerolimov commented Sep 30, 2020 •

edited

jerolimov commented Oct 2, 2020 •

edited