Skip to content

fix(migration): panic when ExecuteMigration fails#8070

Merged
abeizn merged 2 commits into
mainfrom
debug-1
Sep 20, 2024
Merged

fix(migration): panic when ExecuteMigration fails#8070
abeizn merged 2 commits into
mainfrom
debug-1

Conversation

@d4x1
Copy link
Copy Markdown
Contributor

@d4x1 d4x1 commented Sep 19, 2024

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

  • I have read through the Contributing Documentation.
  • I have added relevant tests.
  • I have added relevant documentation.
  • I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

What does this PR do?
Panic when ExecuteMigration fails.

Why?

ExecuteMigration is regared as a function that will succeed always, because if it return an error, backend will panic directly.
image

What problems can be solved?

In these code:

    router.GET("/proceed-db-migration", func(ctx *gin.Context) {
		// Execute database migration
		err := services.ExecuteMigration()
		if err != nil {
			// Return error response
			shared.ApiOutputError(ctx, errors.Default.Wrap(err, "error executing migration"))
			return
		}
		// Return success response
		shared.ApiOutputSuccess(ctx, nil, http.StatusOK)
	})

ExecuteMigration will return an error and the main program will go on.

If there is a new migration script which will return an error when executing , user will have to process db migration.
When this API is called, it returns an error.
But errors are different.
The frist time, it will return from https://github.com/apache/incubator-devlake/blob/main/backend/server/services/init.go#L157.
The second and following times, it will return from https://github.com/apache/incubator-devlake/blob/main/backend/server/services/init.go#L146.

Users can never handle this expect for a reboot.

I think it's unreasonable and this PR just try to fix this.

Does this close any open issues?

There are some related issues #8018 #7992 , but this fix is not the root cause of them.

Screenshots

Include any relevant screenshots here.

It will not return There is a migration in progress. The backend will panic and recover(by gin framework if it's in release mode) directly.

image

Other Information

Any other information that is important to this PR.

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. component/framework This issue or PR relates to the framework pr-type/bug-fix This PR fixes a bug severity/p1 This bug affects functionality or significantly affect ux labels Sep 19, 2024
@abeizn abeizn merged commit 6f0e0db into main Sep 20, 2024
@abeizn abeizn deleted the debug-1 branch September 20, 2024 01:58
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Sep 20, 2024
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Target: #release-v1.0 cherry pick finished successfully 🎉!

@github-actions github-actions Bot added the bot/auto-cherry-pick-completed auto cherry pick completed label Sep 20, 2024
d4x1 added a commit that referenced this pull request Sep 20, 2024
Co-authored-by: Lynwee <1507509064@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot/auto-cherry-pick-completed auto cherry pick completed component/framework This issue or PR relates to the framework lgtm This PR has been approved by a maintainer needs-cherrypick-v1.0 pr-type/bug-fix This PR fixes a bug severity/p1 This bug affects functionality or significantly affect ux size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants