Bug 1924143: Create application edit url based on git provider #8028

rottencandy · 2021-02-02T17:10:01Z

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

Analysis / Root cause:
There is no standard way for git providers to display a repository's branch.

Solution Description:

Craft the git url based on the git provider, and resort to not using branch name if the git provider is unsupported.
```
GitHub: {repo}/tree/{branch}
GitLab: {repo}/-/tree/{branch}
Bitbucket: {repo}/src/{branch}
```
Avoid adding master in import flow if branch is not provided, as this fails for branches whose default branch is not master.

Unit test coverage report:
Added tests.

$ jest /data-transforms/__tests__/data-transformer
 PASS  packages/topology/src/data-transforms/__tests__/data-transformer.spec.ts (8.066s)
Test Suites: 1 passed, 1 total
Tests:       1 skipped, 25 passed, 26 total
Snapshots:   0 total
Time:        9.822s
Ran all test suites matching /\/data-transforms\/__tests__\/data-transformer/i.
Done in 11.39s.

Browser conformance:

Chrome
Firefox
Safari
Edge

/kind bug

openshift-ci-robot · 2021-02-02T17:10:08Z

@rottencandy: This pull request references Bugzilla bug 1924143, 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 1924143: Create application edit url based on git provider

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 · 2021-02-02T17:11:37Z

/cc @sahil143 @jerolimov

jerolimov

I like the refactoring and direction but have some small ideas here. Esp. we should support also custom domains when possible.

jerolimov · 2021-02-02T20:09:12Z

frontend/packages/dev-console/src/utils/resource-label-utils.ts

  return {
    'app.openshift.io/vcs-uri': gitURL,
-    'app.openshift.io/vcs-ref': ref,
+    'app.openshift.io/vcs-ref': gitRef || '',


Why do we not default here to master (or main?) anymore in the annotations? This is not required as part of this change, or?

This creates problems for repos where the default branch is not master. For ex: https://github.com/redhat-developer/s2i-dotnetcore-ex or https://github.com/eggheadio/egghead-next
The edit icon then gets this incorrect branch name and adds it to the url, which leads to 404. I think that not having a branch in url is better than assuming an incorrect branch.
But I can undo this if it is not needed.

Sounds valid for me. You can keep this.

I thought the gitRef was set in these cases. Can you check where we "lose" the branch info in the add flow for these repos? I really would like to see their branch names here.

Looks like we do not try to fetch the the default branch if the ref isn't provided, so it never gets set in the first place:
https://github.com/openshift/console/blob/master/frontend/packages/git-service/src/services/github-service.ts#L83
The git APIs do not require branch to be supplied, and will operate on the default branch.

So unless a ref is provided by user, it will not be set.

ParseBitbucketUrl() seems to provide branch as well which is used for default branch if a ref is not provided (

console/frontend/packages/git-service/src/services/bitbucket-service.ts

Line 37 in a07aa74

const { name, owner, host, branch } = ParseBitbucketUrl(this.gitsource.url);

)

It appears to extract branch name from the URL, and return 'master' if there is no branch.

https://github.com/codefresh-io/parse-bitbucket-url/blob/master/index.js#L189

jerolimov · 2021-02-02T20:12:56Z

frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.ts

+  it('should return full git url for GitHub URI', () => {
+    const mockGitURI = 'https://github.com/openshift/console';
+    const editUrl = getEditURL(mockGitURI, 'branch1');
+    expect(editUrl).toBe('https://github.com/openshift/console/tree/branch1');
+  });
+
+  it('should return full git url for GitLab URI', () => {
+    const mockGitURI = 'https://gitlab.com/example/reponame';
+    const editUrl = getEditURL(mockGitURI, 'branch1');
+    expect(editUrl).toBe('https://gitlab.com/example/reponame/-/tree/branch1');
+  });
+
+  it('should return full git url for Bitbucket URI', () => {
+    const mockGitURI = 'https://bitbucket.org/username/examplerepo';
+    const editUrl = getEditURL(mockGitURI, 'branch1');
+    expect(editUrl).toBe('https://bitbucket.org/username/examplerepo/src/branch1');
+  });
+
+  it('should return only git url for repo from non-supported git provider', () => {
+    const mockGitURI = 'https://example.com/username/examplerepo';
+    const editUrl = getEditURL(mockGitURI, 'branch1');
+    expect(editUrl).toBe('https://example.com/username/examplerepo');
+  });
+


Can you please add also cases for custom domains like github.example.com, gitlab.example.com and bitbucket.example.com? In this cases I also expect the full GitHub branch pathname.

I expect we can not determinate git.example.com, but that's acceptable for me at the moment.

frontend/packages/topology/src/utils/topology-utils.ts

jerolimov

Tested it with GitHub and GitLab and works fine for me. Also know that the URL schema is valid for BitBucket. I noticed that it would also be cool at attach the contextDir if used, but this is another issue. ;)

/lgtm

rottencandy · 2021-02-04T16:01:12Z

/assign @rohitkrai03

rohitkrai03

/approve

openshift-ci-robot · 2021-02-04T18:12:28Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:10:52Z

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

openshift/console#8028

Bugzilla bug 1924143 has been moved to the MODIFIED state.

In response to this:

Bug 1924143: Create application edit url based on git provider

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 · 2021-06-01T06:45:24Z

/cherrypick release-4.6

openshift-cherrypick-robot · 2021-06-01T06:46:11Z

@rottencandy: #8028 failed to apply on top of branch "release-4.6":

Applying: craft git url according to git provider
Using index info to reconstruct a base tree...
M	frontend/package.json
M	frontend/packages/dev-console/src/utils/resource-label-utils.ts
A	frontend/packages/topology/src/data-transforms/__tests__/data-transformer.spec.ts
A	frontend/packages/topology/src/utils/topology-utils.ts
M	frontend/yarn.lock
Falling back to patching base and 3-way merge...
Auto-merging frontend/yarn.lock
Auto-merging frontend/packages/dev-console/src/utils/resource-label-utils.ts
Auto-merging frontend/packages/dev-console/src/components/topology/topology-utils.ts
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/components/topology/topology-utils.ts
Auto-merging frontend/packages/dev-console/src/components/topology/data-transforms/__tests__/data-transformer.spec.ts
Auto-merging frontend/package.json
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 craft git url according to git provider
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:

/cherrypick 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 added kind/bug Categorizes issue or PR as related to a bug. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. labels Feb 2, 2021

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/dev-console Related to dev-console labels Feb 2, 2021

openshift-ci-robot requested review from christianvogt and jeff-phillips-18 February 2, 2021 17:10

openshift-ci-robot requested review from jerolimov and sahil143 February 2, 2021 17:11

jerolimov suggested changes Feb 2, 2021

View reviewed changes

rottencandy force-pushed the topology-editurl branch from a286cf6 to a4a4167 Compare February 3, 2021 07:42

rottencandy requested a review from jerolimov February 3, 2021 07:43

craft git url according to git provider

a4e42e7

rottencandy force-pushed the topology-editurl branch from a4a4167 to a4e42e7 Compare February 3, 2021 12:28

jerolimov approved these changes Feb 4, 2021

View reviewed changes

openshift-ci-robot assigned jerolimov Feb 4, 2021

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

openshift-ci-robot assigned rohitkrai03 Feb 4, 2021

rohitkrai03 approved these changes Feb 4, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2021

openshift-merge-robot merged commit 1ee5a82 into openshift:master Feb 4, 2021

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

rottencandy deleted the topology-editurl branch February 8, 2021 08:09

rottencandy mentioned this pull request Jun 1, 2021

[release-4.6] Bug 1966422: Create application edit url based on git provider #9088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1924143: Create application edit url based on git provider #8028

Bug 1924143: Create application edit url based on git provider #8028

rottencandy commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

rottencandy commented Feb 2, 2021

jerolimov left a comment

jerolimov Feb 2, 2021

rottencandy Feb 3, 2021

jerolimov Feb 3, 2021 •

edited

rottencandy Feb 3, 2021

nemesis09 Feb 3, 2021

rottencandy Feb 4, 2021

jerolimov Feb 2, 2021

rottencandy Feb 3, 2021

jerolimov left a comment

rottencandy commented Feb 4, 2021

rohitkrai03 left a comment

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

rottencandy commented Jun 1, 2021

openshift-cherrypick-robot commented Jun 1, 2021

Bug 1924143: Create application edit url based on git provider #8028

Bug 1924143: Create application edit url based on git provider #8028

Conversation

rottencandy commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

rottencandy commented Feb 2, 2021

jerolimov left a comment

Choose a reason for hiding this comment

jerolimov Feb 2, 2021

Choose a reason for hiding this comment

rottencandy Feb 3, 2021

Choose a reason for hiding this comment

jerolimov Feb 3, 2021 • edited

Choose a reason for hiding this comment

rottencandy Feb 3, 2021

Choose a reason for hiding this comment

nemesis09 Feb 3, 2021

Choose a reason for hiding this comment

rottencandy Feb 4, 2021

Choose a reason for hiding this comment

jerolimov Feb 2, 2021

Choose a reason for hiding this comment

rottencandy Feb 3, 2021

Choose a reason for hiding this comment

jerolimov left a comment

Choose a reason for hiding this comment

rottencandy commented Feb 4, 2021

rohitkrai03 left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

rottencandy commented Jun 1, 2021

openshift-cherrypick-robot commented Jun 1, 2021

jerolimov Feb 3, 2021 •

edited