-
-
Notifications
You must be signed in to change notification settings - Fork 293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: bitbucket cloud url processing for commit and PR links #775
base: master
Are you sure you want to change the base?
chore: bitbucket cloud url processing for commit and PR links #775
Conversation
@FilipSwiatczak what about https:// urls? |
hi @mathpaquette , the bitbucket ssh url is almost identical to the github one with the distinction of having .git at the end, therefore the:
Is a word for word copy of the previous handleSshUrl with the exception of regex including above .git. |
} from '@sorry-cypress/dashboard/lib/github'; | ||
|
||
export const getCommitURL = (repo: string, sha: string) => | ||
repo.includes('bitbucket.org') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting this code not to be specific to bitbucket since it's in repository.ts file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, just a bit sad that it will just work with bitbucket cloud. pretty sure its the same logic needed for on prem version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the repository.ts is just an idea of a switchboard so that other parts like commit.tsx call unified getCommitUrl() etc. and let repository.ts decide which repo type we're dealing with.
I don't have a BB server but it would be straight forward to add another bitbucketServer.ts and another entry in the repository.ts down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FilipSwiatczak yeah but a cleaner way of doing it would be, each implementation should provide one canHandle method and we create an array with all supported implementation (in repository.ts) and we iterate through them til we find supported one.
@agoldis are you okay with those changes ?
@mathpaquette @FilipSwiatczak thanks for reviewing it, if you guys think it's a good PR and no other comments please feel free to proceed |
References
<here>
<here>
Use case
Dashboard would use a separate function to treat Bitbucket Cloud links to Commits and PRs.
https://bitbucket.org/<org>/<repo>/commits/<hash>
<-- commitShttps://bitbucket.org/<org>/<repo>/pull-requests/76
<-- pull-requests instead of tree, and PR- removedI'm including the Bitbucket update in this PR. There is also new lib/repository.ts that detects if github/bitbucket is being used and returns correct implementation to the commit.tsx
This is basic but easy to read and understand and hopefully easy to extend for others like GitLabs or Bitbucket server.
Example
Documentation: this is an invisible improvement and I didn't see any section on gitbook that would warrant an addition, please correct if I'm wrong.
Tests: I would have written unit tests but the Dashboard does not look to have any test framework added (like jest) and preferred not to introduce too much change.
Bitbucket url to ssh regex should be working:
We are looking forward to using Sorry Cypress with Bitbucket cloud. Thanks!