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

Cannot Update Merged PR status. #72

Open
EminUZUN opened this issue Aug 7, 2023 · 6 comments
Open

Cannot Update Merged PR status. #72

EminUZUN opened this issue Aug 7, 2023 · 6 comments

Comments

@EminUZUN
Copy link

EminUZUN commented Aug 7, 2023

Hi,

When I try to merge pull request, the job succeeds it commits changes to main repo but It does not change status of pr to merged.
When I click "merge" again. job succeeds each time.
image

image

@coffeegoddd
Copy link
Contributor

@EminUZUN I apologize for the late response here, I was out sick all last week. Thanks for the report.

Do the jobs make successful commits to main each time, or did only the first successful job make a commit on main?

I would expect that the merged data appears in main only once, so if you are seeing multiple commits on main related to this identical merge, please let me know, and provide the diff it shows.

There are a couple things that might be happening here that might not be working as expected.

First, when a Merge Job is triggered a new Docker container is created that runs dolt merge, merging the target branch into the base branch. Then, the Job runs dolt push, pushing the updated base branch to the remote. The Merge Job actually ends here.

Next, when DoltLab's Remote API receives a push, it calls an internal RPC in DoltLab's API that attempts to reconcile the state of all pull requests for the branches involved in the merge.

It appears that this internal RPC (after the first Merge Job succeeded) failed to reconcile the state of PR #742. I will try to determine how this might have occurred and add some additional logs within the internal RPC to give us better clues about what is happening.

If my expectation is correct that the first Merge Job for PR #742 actually performed the merge and updated main once correctly (although it still failed to change the PR state to merged), then the subsequent Merge Jobs actually succeeded when they should have failed.

I believe these subsequent Jobs ran dolt push as their final step, which would produce Everything up-to-date, indicating that no new data was actually pushed, but this should be considered a Merge Job failure, where now, it is considered a Merge Job success.

I will make this fix to the Merge Job and schedule a DoltLab release.

In the meantime, you can manually change the state of the pull request using the shell-db.sh script and executing the following:

select id from repositories where name = <db name>; # retrieve the id of the database
update pulls set state = 3 where repository_id_fk = <repository id> and id = 742; # update pull 742's state to merged

@coffeegoddd
Copy link
Contributor

Please try the latest DoltLab, it has some fixes for the Jobs, and additional logs.

@EminUZUN
Copy link
Author

Do the jobs make successful commits to main each time, or did only the first successful job make a commit on main?
All jobs are successful. But only first one is commited.

I made a manual remote gc. On new PR's I don't have this issue. But we will deploy the fix a.s.a.p.

@EminUZUN
Copy link
Author

EminUZUN commented Sep 28, 2023

Hey @coffeegoddd,

We have this issue still. I have checked with 1.1.1 today.

Original PR is being merged. But status is not changing.
I am clicking to merge again, makes empty commit and I can see it in commit history.

I need to update status from database. (As when fork is deleted pull activity is being deleted also).

@coffeegoddd
Copy link
Contributor

@EminUZUN can you please share the commit log showing the empty commit from the second merge job?

Also, so I can try to reproduce this bug on my end, can you share the size of the database, the number of open PRs and merged PRs (this is visible on the database list page).

Also can you email the doltlab_doltlabapi_1 logs to me ([email protected]). You can copy the file specified in LogPath when you run docker inspect doltlab_doltlabapi_1.

@EminUZUN
Copy link
Author

@EminUZUN can you please share the commit log showing the empty commit from the second merge job?

Also, so I can try to reproduce this bug on my end, can you share the size of the database, the number of open PRs and merged PRs (this is visible on the database list page).

Also can you email the doltlab_doltlabapi_1 logs to me ([email protected]). You can copy the file specified in LogPath when you run docker inspect doltlab_doltlabapi_1.

This is second merge.
image

Size of db 5.58 mb
Count of pr 160 (All is merged, but almost every PR is manually closed in this database)
DDL:
CREATE TABLE p_definition (
id varchar(36) NOT NULL DEFAULT (uuid()),
x varchar(255) NOT NULL,
y text,
xml_resource text,
z int NOT NULL,
PRIMARY KEY (id),
CONSTRAINT check_uuid_id CHECK (is_uuid(id))
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin
xml_resource has large xml files.
Btw we are not able to insert or update xml_resource from doltlab UI due to header size limitations.
I will try to prepare and send you logs via email.

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

No branches or pull requests

2 participants