Skip to content
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.

Sign up for GitHub

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

Jump to bottom

JENKINS-48737 Fixing lightweight pull request checkouts, also for files in subdirectories #174

Merged
merged 9 commits into from
Apr 7, 2019
Merged

JENKINS-48737 Fixing lightweight pull request checkouts, also for files in subdirectories #174

merged 9 commits into from
Apr 7, 2019

Conversation

seb-lie
Copy link
Contributor

@seb-lie seb-lie commented Mar 1, 2019

Fixing lightweight checkout for PRs for merge and head revisions. The easiest way seemed to pull more data (SCMHead) into BitbucketSCMFile to be able to distinguish between merge and head PRs in a central location.

Validated on Atlassian Bitbucket Server (DC) v5.8.2 for Jenkinsfiles as well as arbitrary files directly in a repo as well as in subdirectories.

There's still the corner case left for lightweight checkouts on conflicted PRs - these are still checked out in full, also on Jenkins masters.

@seb-lie
Copy link
Contributor Author

seb-lie commented Mar 1, 2019

Suggested fix for https://issues.jenkins-ci.org/browse/JENKINS-48737.

@jetersen
Copy link
Member

jetersen commented Mar 1, 2019

@seb-lie I think this could be better implemented:
See: #117 (comment)
#117 (comment)

jetersen
jetersen requested changes Mar 1, 2019
View reviewed changes
...ain/java/com/cloudbees/jenkins/plugins/bitbucket/server/client/BitbucketServerAPIClient.java Outdated
@@ -147,7 +147,7 @@ public BitbucketCommit call() throws Exception {
private static final String API_PULL_REQUESTS_PATH = API_REPOSITORY_PATH + "/pull-requests{?start,limit,at,direction,state}";
private static final String API_PULL_REQUEST_PATH = API_REPOSITORY_PATH + "/pull-requests/{id}";
private static final String API_PULL_REQUEST_MERGE_PATH = API_REPOSITORY_PATH + "/pull-requests/{id}/merge";
private static final String API_BROWSE_PATH = API_REPOSITORY_PATH + "/browse{/path*}{?at}";
private static final String API_BROWSE_PATH = API_REPOSITORY_PATH + "/browse{/path*}?at={+at}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use handy uri as intended. the ?at makes at optional.
if your worried about quoting there is a way to fix that in handy uri, I suggest you read the docs :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! Yes, I was concerned that forward slashes were escaped which broke the path in subdirs. I looked through rfc6570 but could not find an example or spec of an optional parameter with a path. Probably I'm missing the obvious - do you have something in mind that enables an expansion like ?at=refs/pull-requests/1184/from ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?at=refs/pull-requests/1184/from should be safe already though. We are not talking about the path part of the URL where you should be concerned. We are talking about the parameter/query part and slashes are perfectly fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if anything that would break subdirs it's the {/path*} part of the
API_BROWSE_PATH

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me test this in a couple of hours so that I can revisit my reasoning for this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a test that verifies the issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API endpoint has been wrong the whole time 😆

Suggested change
private static final String API_BROWSE_PATH = API_REPOSITORY_PATH + "/browse{/path*}?at={+at}";
private static final String API_BROWSE_PATH = API_REPOSITORY_PATH + "/browse/{+path}{?at}";

Did some testing:

The current template that is wrong expands to this

http://bitbucket/rest/api/1.0/projects/test/repos/test/browse/folder%2FJenkinsfile?at=fix%2Ftest

The new template that is correct expands to this

http://bitbucket/rest/api/1.0/projects/test/repos/test/browse/folder/Jenkinsfile?at=fix%2Ftest

This template /browse/{+path}?at={+at} works too and it expands to

http://bitbucket/rest/api/1.0/projects/test/repos/test/browse/folder/Jenkinsfile?at=fix/test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some test locally that I will push soon

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFile.java Outdated Show resolved Hide resolved
jetersen
jetersen reviewed Mar 3, 2019
View reviewed changes
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somie minor comments

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFileSystem.java Outdated Show resolved Hide resolved
@jetersen jetersen mentioned this pull request Mar 3, 2019
Merged
@bitwiseman
Copy link
Contributor

related: jenkinsci/github-branch-source-plugin#209

@jetersen jetersen force-pushed the fix_lightweight_checkouts_2.4.2 branch from 54002da to cacc5e7 Compare April 6, 2019 22:04
jetersen
jetersen reviewed Apr 6, 2019
View reviewed changes
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFileSystem.java
if (pr.getCheckoutStrategy() == ChangeRequestCheckoutStrategy.MERGE) {
return null;
} else if (pr.getCheckoutStrategy() == ChangeRequestCheckoutStrategy.HEAD) {
ref = pr.getOriginName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing out whether this is even possible

Copy link
Member

@jetersen jetersen Apr 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup only possible when the PR is coming from the same repository.
Atlassian is the one to blame for having an issue open for six years and counting.

@jetersen jetersen merged commit 868676d into jenkinsci:master Apr 7, 2019
@jetersen jetersen added the fix label Apr 7, 2019
This was referenced Apr 7, 2019
Closed
Closed
@mkjkec2005
Copy link

Hi,

This does not work for me. The Jenkins falls back to heavy weight checkout when using a Bitbucket Team project.

Jenkins version: [2.391]
Plugin version: [796.v6cb_1559e1673]
Bitbucket server version: 7.21.6

Regards,
Mohan Krishan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jetersen jetersen jetersen approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seb-lie @jetersen @bitwiseman @mkjkec2005