Skip to content

Conversation

@pserwylo
Copy link
Contributor

I noticed in another PR of mine that the performance-8.0 test was failing, where it was passing for many other PRs. On inspection, there were others PRs where this failed, and all of them were where the PR came from a fork rather than a branch in this repo.

This is because there is an assumption in the workflow that the PR is from the same origin as the base it is being merged into. This is fixed so that instead of doing a git fetch origin, it now fetches from the clone_url of the PR head.

Subsequent to fetching commits from that repository, it then clones based on sha1 instead of branch name. This is because we didn't name the remote when performing a fetch, so can't use the syntax remote-name/ref (e.g. "myusername/fix-issue-12").

We could equally go and add a remote before fetching, then fetch from that remote (e.g git add remote head ... && git fetch head) which would then let us git merge head/${{ ref }} - it doesn't really matter which way we do it.

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Thanks

@szaimen szaimen added bug 3. to review Waiting for reviews labels Nov 16, 2022
@szaimen szaimen requested review from a team and icewind1991 November 16, 2022 06:53
@szaimen szaimen added this to the Nextcloud 26 milestone Nov 16, 2022
@skjnldsv
Copy link
Member

From https://github.com/pserwylo/nextcloud-server
 * branch                  HEAD       -> FETCH_HEAD
warning: 9d07ec5a96d7995785f6d3d377024ba09977cb91:.gitmodules, multiple configurations found for 'submodule.3rdparty.path'. Skipping second one!
warning: 9d07ec5a96d7995785f6d3d377024ba09977cb91:.gitmodules, multiple configurations found for 'submodule.3rdparty.url'. Skipping second one!
Fetching submodule 3rdparty
fatal: remote error: upload-pack: not our ref 03c3817ff[13](https://github.com/nextcloud/server/actions/runs/3475945299/jobs/5812897563#step:7:14)2653c794fd04410977952f69fd6[14](https://github.com/nextcloud/server/actions/runs/3475945299/jobs/5812897563#step:7:15)
Errors during submodule fetch:
	3rdparty

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 16, 2022
@pserwylo
Copy link
Contributor Author

Thanks for kicking the builds off (first time contributor so they wont kick off automatically). Now that they've run and we can see that error, I'll tweak then amend the commit, which should kick off a new build, then comment back once it is building as expected.

@pserwylo pserwylo force-pushed the fix-performance-github-action branch from d9372cc to ba88faf Compare November 16, 2022 11:17
@pserwylo
Copy link
Contributor Author

Oh gosh, it needs approval every time I make a change. Let me go try forking my fork, so that I can simulate this without having to bother you all. Will report back when I am more confident in the solution.

@pserwylo pserwylo force-pushed the fix-performance-github-action branch from ba88faf to 753ef54 Compare November 16, 2022 12:47
@pserwylo
Copy link
Contributor Author

Okay, have tried this with a fork-of-a-fork requesting a PR to my fork, and it now should work as expected. Code is more explicit and probably what I would do if I was typing the commands out manually (create a new remote, and use that remote when fetching and referring to remote refs).

There is an assumption that the PR is from the same remote as
the base it is being merged into. This is fixed so that instead
of doing a `git fetch origin`, it now fetches from the `clone_url`
of the PR head.

Signed-off-by: Peter Serwylo <[email protected]>
@pserwylo pserwylo force-pushed the fix-performance-github-action branch from 753ef54 to 56aee55 Compare November 16, 2022 12:50
@pserwylo
Copy link
Contributor Author

Phew, think we're doing the right thing now, hopefully good to go:

https://github.com/nextcloud/server/actions/runs/3479487910/jobs/5818120859

image

@juliusknorr juliusknorr merged commit b62d3c4 into nextcloud:master Nov 16, 2022
@welcome
Copy link

welcome bot commented Nov 16, 2022

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@juliusknorr
Copy link
Member

Thanks a lot for that @pserwylo 👍

@nickvergessen
Copy link
Member

/backport to stable25

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

Labels

2. developing Work in progress bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants