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.

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

Fix PR info passed and delete shadow merge #75

Merged
merged 1 commit into from Jul 15, 2021

Conversation

ivan1767p
Copy link
Contributor

Fix PR info passed to TeamCity build. I think shadow merge no need. Because we can merge any PR in build TC.

Tested environment:
TeamCity Professional 2020.2.4 (build 86063)
Atlassian Bitbucket v7.6.3

@jmecosta
Copy link
Collaborator

The shadow merge was a feature provided by some other user, see #70. Can't you fix in a way that shadow merge remains?

@ivan1767p
Copy link
Contributor Author

ivan1767p commented Jul 14, 2021

What for? We have information about PR in TeamCity. If we want merge, ok. Just create "command line" step in TeamCity.

@jmecosta
Copy link
Collaborator

That means u delegate that functionality for users to deal with when it can be provided straight from the hook side?

I mean I understand everything can be done I'm teamcity side, but that doesn't count as a feature from this hook right?

@dzwicker wanna share your opinion, since this is your use case. I can revert it if you agree with @ivan1767p

@ivan1767p
Copy link
Contributor Author

ivan1767p commented Jul 14, 2021

I mean I understand everything can be done I'm teamcity side, but that doesn't count as a feature from this hook right?
No. We need PR info. Example,

# Command line step from TeamCity
git fetch origin refs/pull-requests/%bitbucket.pr.number%/from:pr-%bitbucket.pr.number%
git checkout pr-%bitbucket.pr.number%
git format-patch %bitbucket.pr.target.branch% --stdout > ../pr-%bitbucket.pr.number%.path
git checkout %bitbucket.pr.target.branch%
git apply ../pr-%bitbucket.pr.number%.path --check > ../checkmerge.txt
if [ $? != 0 ]; then
  echo "Detect merge conflicts"
  exit 1
else
  git merge pr-%bitbucket.pr.number% > ../result.txt
  cat ../result.txt
fi

In version 4.10.1 PR info passed is broken.

@jmecosta
Copy link
Collaborator

Well in that case this is also redundant, the hook was earlier already sending that info via comment to teamciy we are using this since start. Also u can get the pr ID using bitbucket rest api you just need really the branch.

And all this assumes that you are doing client side checkout, server side checkout you are left hanging.

But yeah, as said, lefts just get opinion of @dzwicker if his fine with it, let's revert this then

@ivan1767p
Copy link
Contributor Author

ivan1767p commented Jul 14, 2021

Well in that case this is also redundant, the hook was earlier already sending that info via comment to teamciy we are using this since start.
How get PR id in step TeamCity? Parameters build TeamCity without PR id. PR id only in comment.

@jmecosta
Copy link
Collaborator

What we do we use the teamciy rest api to get all that information. All that is avaliable if you use curl or whatever. The point is your method is a more clean way of doing it, I can see that. However I can see also that shadow merge from the hook side is also cleaner than forcing users to write their own logic on a teamciy step.

@ivan1767p
Copy link
Contributor Author

ivan1767p commented Jul 15, 2021

@jmecosta, about shadow merge. Please see https://jira.atlassian.com/browse/BSERV-12284

refs/pull-requests/*/merge has been fully removed in 7.0 and will never be created/updated by the system again. For pull requests that were opened prior to upgrading to 7.x, which have a merge ref, that ref will be removed the first time the pull request is updated (or when it's merged or declined, as ever). With the switch from 3-way diffs (which were built around displaying the merge's diff) to 2-way diffs (which use the common ancestor), the system no longer retains the merges it creates to check mergeability; it runs git merge to see what happens, and discards everything that results. This prevents the accumulation of "unreachable" pull request merge objects in repositories, which can (greatly) slow down clone/fetch/pull/push performance as they build up. Any tool that wishes to build a pull request's merge will need to create that merge on its own, going forward.

Prior to 7.0, because pull request diffs were built around the merge, checking mergeability (which happens automatically after page load when a pull request is created via the UI) would create the public refs/pull-requests entries for that pull request. In 7.x, since the diff is now using the common ancestor and no longer needs the merge, checking mergeability doesn't create any refs. (This is shown in the log snippets above.) Instead, the refs are created when a pull request's diff is viewed. Plugins which are installed in Bitbucket Server can simulate this by either using the PullRequestService to stream the pull request's changes, or (more efficiently), by using ScmService.getPullRequestCommandFactory(pullRequest).effectiveDiff() (which PullRequestService uses under the hood). Remote callers can use rest/api/1.0/projects/KEY/repos/slug/pull-requests/N/changes?limit=1 to do the same. Any of those will ensure refs/pull-requests/*/from is created (or updated) to match the pull request's current state.

I'd wager many webhook apps (or other apps that notify external systems, however they do so) have logic in them for using PullRequestService.canMerge, MergeRequestCheckService.check or ScmPullRequestCommandFactory.tryMerge to try and force pull request refs to be created/updated. (The Post Webhooks for Bitbucket app did, back when it was open source; the source has since been made private, or at least moved off Github, so I'm not sure these days.) Those approaches will not work in 7.x. Instead, such apps need to be updated to use PullRequestService.streamChanges or ScmPullRequestCommandFactory.effectiveDiff to force the from ref to be created/updated. As noted earlier, there is no way in 7.x to get a merge ref.

I'll reiterate here that Bitbucket Server's pull request refs are not API, and have never been considered API, so the way they work is dangerous to rely on. I'll also readily acknowledge that that's hardly a satisfactory answer, to receive or to give, and that it would be nice if it changed. I am not a PM for Bitbucket Server, so I can't say when/if such improvements will hit the roadmap. Perhaps I'll see if I can use some upcoming 20% time to make some improvements. I'm sorry that, at least for the moment, I can't offer better.

Best regards,
Bryan Turner
Atlassian Bitbucket

#70 not worked for Atlassian Bitbucket v7.x. It's fact.
I'm not sure this #70 worked for Atlassian Bitbucket v6.x

Release 4.10.1 is broken!

@jmecosta
Copy link
Collaborator

@ivan1767p alright, then all good for me to revert. thanks for the reference ticket.. that i supose explains why it didnt work either in your previous email

@jmecosta jmecosta merged commit 1962b6a into TrimbleSolutionsCorporation:master Jul 15, 2021
@jmecosta
Copy link
Collaborator

@ivan1767p thanks for this, its now release in marketplace. https://marketplace.atlassian.com/manage/apps/1215178/versions/300000270/details

@ivan1767p
Copy link
Contributor Author

@jmecosta , thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants