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

Manager refactor, part 1 #4298

Merged
merged 9 commits into from
Nov 28, 2023

Conversation

DingDongSoLong4
Copy link
Collaborator

The time has come for more refactoring.

This PR mostly deals with the initialization process. In general, the whole process is now a lot simpler and more straightforward.

The main function in cmd/stash/main.go is now a lot cleaner. A channel is now used to exit with an exit code, rather than just calling os.Exit in Manager.Shutdown. This means that everything can be properly shut down using defers.

I've also moved a few things around - the manager initialization code is now in a separate init.go file for example. And some renaming: config.Instance to config.Config and Manager.FFMPEG to Manager.FFMpeg.

And then a small fix to the Makefile after #4153, to prevent "nothing to be done" warnings even when specifying multiple flags-* targets (e.g. make flags-release flags-pie stash).

@WithoutPants
Copy link
Collaborator

Setup is broken with these changes. I'm getting a crash due to concurrent map read/write access in the viper code. If I then restart stash and retry the final setup step, it returns a fetch error. This may be an existing latent bug when trying to re-setup the system after it was created (since the config.yml was created prior to the crash).

@DingDongSoLong4
Copy link
Collaborator Author

I wasn't able to reproduce the crash myself, but I was able to find a data race which I assume was the cause with the help of -race. I was also getting the same data race warning on develop - the bug appears to have been there for a while.

And then the setup error I believe was simply that the error state in the Setup component wasn't being reset after clicking "Back" - so if an error occurs the first time it sticks around and you can't continue, even if no more errors occur.

@WithoutPants WithoutPants added the chore Pull requests for refactoring and admin work label Nov 28, 2023
@WithoutPants WithoutPants added this to the Version 0.24.0 milestone Nov 28, 2023
@WithoutPants WithoutPants merged commit b78771d into stashapp:develop Nov 28, 2023
@DingDongSoLong4 DingDongSoLong4 deleted the manager-refactor branch November 28, 2023 13:06
halkeye pushed a commit to halkeye/stash that referenced this pull request Sep 1, 2024
* Move BackupDatabase and AnonymiseDatabase to internal/manager
* Rename config.Instance to config.Config
* Rename FFMPEG
* Rework manager and initialization process
* Fix Makefile
* Tweak phasher
* Fix config races
* Fix setup error not clearing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for refactoring and admin work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants