search

getumbrel / umbrel-apps

The official app repository of the Umbrel App Store. Submit apps and updates here. Learn how → https://github.com/getumbrel/umbrel-apps#readme
https://apps.umbrel.com
480 stars 357 forks source link

App Submission: Samba #1161

Open joaovictor-local opened 1 week ago

joaovictor-local commented 1 week ago

I am already using it through a community store (https://github.com/joaovictor-local/umbrel-samba-app-store) but it requires me to change the password manually editing the files using SSH, before that I was using Samba directly installed on the host but it was deleted by a system update so I had to figure out how to eliminate this problem.

I think it need at least to generate a random password for each installation, maybe a script to generate the password and insert it at the /data files and the umbrel-app.yml :heavy_check_mark: our allow the user to sign in using his Umbrel password (I not sure if it's possible on Linux)

What do you guys think about the idea of this app? A web app to edit the config file and password would be cool too and I think I could code it. If we it would be possible to use the same password we would just need a better index.html.

Icon: https://github.com/getumbrel/umbrel-apps-gallery/pull/47

joaovictor-local commented 1 week ago

Hi @joaovictor-local,

thank you so much for your awesome app submission, I am really impressed! I also think this app is needed as umbrelOS currently has no app to export its storage via samba.

I have left a couple of suggestions below. If you have any questions, feel free to ask me anytime.

Note: I haven't tested the app yet, but I will do that as soon as you have updated this PR 😀

Thanks again for your awesome work, I can't wait to see your changes!

Hi @sharknoon! I appreciate! Thank you for you review :smile: I applied all your requests.

image

joaovictor-local commented 1 week ago

Wow, awesome work! You are really fast!

I have tested it on my umbrel and it worked flawlessly. I think I will keep this app on my personal umbrel :)

I have made some suggestions below. It can happen, that the user change (user: "1000:1000") in the docker compose file will not work. Please test if it workd with less permissions and let me know.

Thank you again :)

I am excited to see it available in the store :smile: It worked fine with less permission for the Static Web Server too :+1:

joaovictor-local commented 1 week ago

@nmfretz @lukechilds @mayankchhabra @smolgrrr :eyes:

nmfretz commented 5 days ago

Hey @joaovictor-local, sorry for the delay on my end. I will look at this tomorrow!

nmfretz commented 4 days ago

Wonderful addition @joaovictor-local! And fantastic review @sharknoon. I'm super impressed with both of you here 🤝

@joaovictor-local this is working really well. We'll start getting gallery assets together so this can go live!

In the meantime, there are a couple things you may want to do to make app updates easier for you:

1) Include the public assets (favicon/index.html/samba.svg) within the homepage Docker image itself.

Ideally, the best thing to do is have your frontend code be part of the Docker image so that frontend app updates are as easy as changing the image key in the compose file to your new image.

The way umbrelOS currently works is that on app install, everything from the app's repo is copied over to a users umbrel data. But on app updates only a whitelist of files is copied over so that user data is not overwritten: https://github.com/getumbrel/umbrel/blob/49d37581c8233f808fc5a79c2cd38810c6cd1b0b/packages/umbreld/source/modules/apps/legacy-compat/app-script#L594-L595

With the current configuration, if you made changes to your UI (e.g., changed the instructions in the html), only new installs would receive the updates. Existing installs that update won't get the changes. You could do some hacky things like use a hook that runs on app update and changes certain files, but it's not ideal because it will add complex overhead for you.

Instead, what you can do is just build your own image from joseluisq/static-web-server that includes your public folder and then push it to DockerHub.

Something as simple as this will likely work for a Dockerfile:

# Use the joseluisq/static-web-server as the base image
# You'll want to update this tag to the latest version each time you build
FROM joseluisq/static-web-server:2.32.0

# Copy your entire public directory to the /var/public directory which is set by the SERVER_ROOT env var
COPY public /var/public

You'll then need to make sure to build a multi-architecture image for amd64 and arm64. You'll run something like this:

docker buildx build --platform linux/amd64,linux/arm64 -t your-docker-hub-username/static-web-server-samba:version-tag .

2) Can the Samba container create it's own smb.conf and we still bind to it?

It might be worth seeing if it's possible to let the Samba container create it's own smb.conf on initial startup, still have us bind to its parent directory, but also allow us to set the things we need to change via environment variables (e.g., force user).

I suspect this won't be possible, so please don't do too much digging. The thing I'm trying to avoid here is again related to app updates not bringing down changes we may need to make to this file at some point down the road.

I'm going to make a couple small changes to the compose file and push them.

nmfretz commented 4 days ago

Alrighty @joaovictor-local I made two main changes:

1) I changed the smb.conf bind mount path on the host to ${APP_DATA_DIR}/data/samba/smb.conf https://github.com/getumbrel/umbrel-apps/pull/1161/commits/541476aefb242163dfba7eab5c45ab4bd8f41539

2) I changed the SMB port from 445 to 446. This is needed because the Back That Mac Up app is already using 445. If a user goes to install both apps then the second one that is being installed will fail because port 445 will already be in use.

This means that the instructions in the `index.html` will need to change to reference `smb://umbrel.local:446`
sharknoon commented 1 day ago

@joaovictor-local 👀

joaovictor-local commented 13 hours ago

@joaovictor-local 👀

I will be doing it today yet (UTC-3). 🙂

joaovictor-local commented 11 hours ago

@sharknoon the only problem with the docker-compose.yml edit it that it will be erased after a update and the user may forget that we need to put it back manually. I have changed the force user and force group keys just for informative proposal because it will always use the values in the environment variables.

sharknoon commented 9 hours ago

@joaovictor-local thank you for your commits 😀 Yes, you are right, changes to the docker-compose.yml will be erased after an update.

smb.conf

I am thinking do we need to have a smb.conf exposed to the user? What are the use cases for a user to edit this file? I am not an expert in Samba, maybe you can clarify it a bit for me, thanks! 😊

Container Image

Would it be possible for you to pack the UI part inside a container as @nmfretz suggested it? That way it is easier to update and maintain. We would also have a clear line of responsibilities (everything feature/app releated inside the container, everything hosting/umbrel related in the data directory/docker compose).

Updated Port

Other than that we had to change to port from the standard 445 to 446, because that port was already allocated by Back That Mac Up. Therefore we also need to change the port in the UI.

Thank you so much for the great work you are doing and I can't wait to see this app on the app store!

joaovictor-local commented 8 hours ago

@joaovictor-local thank you for your commits 😀 Yes, you are right, changes to the docker-compose.yml will be erased after an update.

smb.conf

I am thinking do we need to have a smb.conf exposed to the user? What are the use cases for a user to edit this file? I am not an expert in Samba, maybe you can clarify it a bit for me, thanks! 😊

Container Image

Would it be possible for you to pack the UI part inside a container as @nmfretz suggested it? That way it is easier to update and maintain. We would also have a clear line of responsibilities (everything feature/app releated inside the container, everything hosting/umbrel related in the data directory/docker compose).

Updated Port

Other than that we had to change to port from the standard 445 to 446, because that port was already allocated by Back That Mac Up. Therefore we also need to change the port in the UI.

Thank you so much for the great work you are doing and I can't wait to see this app on the app store!

  1. You may want to change it to make it faster. https://www.reddit.com/r/OpenMediaVault/comments/11gwi1g/significant_samba_speedperformance_improvement_by/

  2. I was working on it but I had to take a break. Take a lot in the last commit and also: https://github.com/joaovictor-local/static-web-server-samba and https://hub.docker.com/repository/docker/loficafe/static-web-server-samba/general.

  3. @nmfretz changed it for me :)