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

New Script: Maintainerr #652

Closed
wants to merge 4 commits into from

Conversation

caroipdev
Copy link

Note

We are meticulous when it comes to merging code into the main branch, so please understand that we may reject pull requests that do not meet the project's standards. It's never personal. Also, game-related scripts have a lower chance of being merged.

Description

Provide a summary of the changes made and/or reference the issue being addressed.

Fixes #97

Type of change

Please check the relevant option(s):

  • Bug fix (non-breaking change that resolves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (a fix or feature that would cause existing functionality to change unexpectedly)
  • New script (a fully functional and thoroughly tested script or set of scripts.)

Prerequisites

The following efforts must be made for the PR to be considered. Please check when completed:

  • Self-review performed (I have reviewed my code, ensuring it follows established patterns and conventions)
  • Testing performed (I have tested my changes, ensuring everything works as expected)
  • Documentation updated (I have updated any relevant documentation)

Additional Information (optional)

Provide any additional context or screenshots about the feature or fix here.

Related Pull Requests / Discussions

If there are other pull requests or discussions related to this change, please link them here:

  • Related PR #

@github-actions github-actions bot added new script A change that adds a new script website A change to the website labels Dec 3, 2024
@benscobie
Copy link

Hi, I'm a contributor for Maintainerr. We can definitely make this script a lot simpler. Currently we only build for Docker but I see no reason why we can't build artifacts for platforms like linux-x64. I'll see if I can make some time to look at this.

With the script the way it is, there's a high chance it'll break in the future as we move things around.

@caroipdev
Copy link
Author

Currently we only build for Docker but I see no reason why we can't build artifacts for platforms like linux-x64. I'll see if I can make some time to look at this.

I was thinking in having a look at the Maintainerr repo to see if it would be possible to add the built files for linux-x64, building both app & server takes quite a lot of time & resources (more than 1GB RAM).

I've tried to keep this script and the generated folder the closest to DockerFile as possible, hopefully that would reduce the amount of work needed if Maintainerr moves things around.

@benscobie
Copy link

I've tried to keep this script and the generated folder the closest to DockerFile as possible, hopefully that would reduce the amount of work needed if Maintainerr moves things around.

The Dockerfile will be getting quite a big change soon with this PR: jorenn92/Maintainerr#1369

I'll look at adding this into there.

@MickLesk
Copy link
Member

MickLesk commented Dec 3, 2024

Hi, I'm a contributor for Maintainerr. We can definitely make this script a lot simpler. Currently we only build for Docker but I see no reason why we can't build artifacts for platforms like linux-x64. I'll see if I can make some time to look at this.

With the script the way it is, there's a high chance it'll break in the future as we move things around.

Hello,

thanks for your feedback, the script has given me a bit of a headache in terms of maintenance.
I think a linux%64 solution is much better, it can be integrated properly and almost error-free.

@caroipdev
Copy link
Author

the script has given me a bit of a headache in terms of maintenance.

I still believe it will still be a headache because we still need to do the db migrations and move files around, it will simplify but not be "almost error-free" :D

@benscobie
Copy link

benscobie commented Dec 3, 2024

I still believe it will still be a headache because we still need to do the db migrations and move files around, it will simplify but not be "almost error-free" :D

This hopefully won't be the case. It should look similiar to some of the other scripts; download and extract archive, data folder creation, chmod and service creation. Database migrations are automatically run on startup.

@caroipdev
Copy link
Author

Database migrations are automatically run on startup.

Great, thanks, was having the error the database settings didn't exist, added Environmnet=UV_USE_IO_URING=0 and it seems to work now :)

@MickLesk please let me know how I can improve this, I'll remove the DB migration code, and will try to tidy up the file moving, the building can be replaced/removed when Maintainerr provides the built files

@MickLesk
Copy link
Member

MickLesk commented Dec 3, 2024

I would probably close the PR until Maintainerr provides a linux file. Because then the script might only have 10-15 lines of main code. (like nextpvr or other deb / linux installations)

@caroipdev
Copy link
Author

I would probably close the PR until Maintainerr provides a linux file. Because then the script might only have 10-15 lines of main code. (like nextpvr or other deb / linux installations)

Up to you, I personally would prefer having something working until the Maintainerr devs have time to work on that and then update the script, instead of not having anything and then having the script created "from scratch" when Maintainerr provides the built files.

Just commited my latest changes, locally it seems to be working, just pushed for future reference :)

@caroipdev caroipdev closed this Dec 3, 2024
@caroipdev caroipdev deleted the maintainerr branch December 3, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new script A change that adds a new script website A change to the website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants