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

(refactor): startapp.sh script #110

Merged
merged 8 commits into from
Feb 7, 2024
Merged

Conversation

xela1
Copy link
Contributor

@xela1 xela1 commented Feb 5, 2024

Found a few issues with this if someone wants to confirm

  1. log_message function was within the 'if client already exists' context, so couldn't be called from the doesn't exist else
  2. on first install from scratch, the log location doesn't exist yet
  3. on first install from scratch, it was attempting to download the installer to / which it has no permissions to

@traktuner
Copy link
Collaborator

HEy @xela1
Thanks for the PR!
When I have time I'll review the changes (might take some time).
Regarding your changes to the path:
Don't write the installer to /tmp in the container
It should be in /config/wine on disk (cd right after the mkdir)
The permissions issue won't be fixed and has nothing to do with our implementation. It's the baseimage we're using, which is a very recent one since v1.6 and the permission scheme was tightened (which is good). Some people just need to add USER_ID and GROP_ID to adjust the permissions, for other users it works out-of-the-box.

@xela1
Copy link
Contributor Author

xela1 commented Feb 6, 2024

Don't write the installer to /tmp in the container It should be in /config/wine on disk (cd right after the mkdir)

This is missing in the section where it installs the client if it doesn't already exist, so I just added cd /tmp, but I can alter that

mkdir -p /config/wine/ &&
if [ "$FORCE_LATEST_UPDATE" = "true" ]; then

Whereas it exists in the update code

fetch_and_install() {
cd "$install_exe_path" || handle_error "UPDATER: can't navigate to $install_exe_path"

Thinking about it more, is there a reason why it doesn't just call the fetch_and_install if there is no current installed client?

I have a few 'enhancements' I've being playing with that I'm happy to run past you before I raise a PR for them

  1. If wine is not installed (first launch) deliberately call wineboot -i to initialise it before progressing
  2. checking for mounted devices (following a pattern /drive_d /drive_e etc) and automatically create the symlink (prior to installing/starting backblaze)
  3. Allow users to set a backblaze username and password environment variable and silently install the client

@traktuner
Copy link
Collaborator

Hey!
Thanks. It's currently a code-mix, I just added the autoupdater function and left the existing code as-is since it's working. You can overhaul the function to directly call fetch_and_install also on first launch.

  1. What would be the benefit of adding wineboot for new installs?

  2. that's a good idea, however I'm one of the users who alter drives a lot, so if a symlink already exists or and the mount is not there, nothing should happen. So symlinks should be created and not removed imo. Backblaze is really picky there and just throws a "drive missing" warning if it does not find the correct volume ID.
    So I'm more cautious with automating things because we don't want to harm peoples's backups.

  3. How would you handle the silent install/update?
    Only the MSI package supports silent deployment but only with business deployments (group ID and group token are mandatory for new and existing installs).

@xela1
Copy link
Contributor Author

xela1 commented Feb 6, 2024

  1. It ensures wine is configured and running before proceeding with anything further in the startapp script, for example, it doesn't need the work around I proposed in the log_message function that was attempting to write to the /config/wine directory
  2. My POC code just adds symlinks, it will never remove, if you have a volume mounted in the container following the /drive_x pattern it will create the symlink. It's not necessary, but it means one less manual action on setting up the container
  3. Personal accounts can be passed as a parameter also, I've got it working on a test build at the moment, the only issue I've got at the moment is it doesn't fully start the control panel after installing and has to be manually opened, I'll play around some more

"install_backblaze.exe" -nogui -signinaccount $BB_USER $BB_PASS

image

image

@traktuner
Copy link
Collaborator

Hey,
let's try adding wineboot and symlinks first (and the fetch_and_install optimization to reduce redundant code)
I am not sure about the silent install. Officially, I have the info from support that this is only available with the MSI package (plus the organization-specific parameters).
I would rather not use undocumented and not officially supported parameters, as it can break anytime.
Would be interesting to hear @JonathanTreffler's opinion on this.

@xela1
Copy link
Contributor Author

xela1 commented Feb 6, 2024

Hey, let's try adding wineboot and symlinks first (and the fetch_and_install optimization to reduce redundant code) I am not sure about the silent install. Officially, I have the info from support that this is only available with the MSI package (plus the organization-specific parameters). I would rather not use undocumented and not officially supported parameters, as it can break anytime. Would be interesting to hear @JonathanTreffler's opinion on this.

The exe help shows the parameters, but I'll leave this for now. I'll submit a PR tomorrow for the other bits for you to review

image

@JonathanTreffler
Copy link
Owner

I would rather not use undocumented and not officially supported parameters, as it can break anytime.

I agree and I am also concerned about 2FA support on the silent install. If accounts with OTP don't work, that would not be acceptable.

@JonathanTreffler
Copy link
Owner

2. Backblaze is really picky there and just throws a "drive missing" warning if it does not find the correct volume ID.

I think Backblaze recognizes drives by a file it writes on the filesystem on the disk. So I think re-adding symlinks would not hurt, but testing of that would be needed.

@xela1
Copy link
Contributor Author

xela1 commented Feb 6, 2024

I would rather not use undocumented and not officially supported parameters, as it can break anytime.

I agree and I am also concerned about 2FA support on the silent install. If accounts with OTP don't work, that would not be acceptable.

Good point, hadn't thought of this, I'll see how it behaves

@xela1
Copy link
Contributor Author

xela1 commented Feb 6, 2024

I think Backblaze recognizes drives by a file it writes on the filesystem on the disk. So I think re-adding symlinks would not hurt, but testing of that would be needed.

It doesn't seem to like the drive letter being changed, the original drive shows as unplugged and doesn't show as the new letter, but when reverted to the original letter it recovers fine.

Adding new disks (following the /drive_x pattern) are in the backblaze client to be selected on the first container startup after adding, no need to restart the container after symlink

@traktuner
Copy link
Collaborator

I would rather not use undocumented and not officially supported parameters, as it can break anytime.

I agree and I am also concerned about 2FA support on the silent install. If accounts with OTP don't work, that would not be acceptable.

Good point, hadn't thought of this, I'll see how it behaves

I asked Backblaze support a 2nd time about this, and they mentioned that this silent install is - even with the exe installer - only available with groupID and group token and will misbehave when those parameters are missing.
2FA would also be a problem.
So a hard no for me.

@traktuner traktuner changed the title Fix client install on new build (refactor): startapp.sh script Feb 7, 2024
@traktuner
Copy link
Collaborator

traktuner commented Feb 7, 2024

I think Backblaze recognizes drives by a file it writes on the filesystem on the disk. So I think re-adding symlinks would not hurt, but testing of that would be needed.

It doesn't seem to like the drive letter being changed, the original drive shows as unplugged and doesn't show as the new letter, but when reverted to the original letter it recovers fine.

Adding new disks (following the /drive_x pattern) are in the backblaze client to be selected on the first container startup after adding, no need to restart the container after symlink

Volume ID and drive letter are "married" for Backblaze. If it sees the drive "X" with a different volume ID it's unplugged for Backblaze.

Adding new symlinks for new mounts is good 👍

@xela1
Copy link
Contributor Author

xela1 commented Feb 7, 2024

I asked Backblaze support a 2nd time about this, and they mentioned that this silent install is - even with the exe installer - only available with groupID and group token and will misbehave when those parameters are missing.

I mean they're incorrect, as it works and has options to do it without the groupID etc, but I'm really not bothered enough to push for this, it was just a nice to have on my list where we can remove as much user interaction as possible

@xela1
Copy link
Contributor Author

xela1 commented Feb 7, 2024

I pushed my latest commits into the PR last night which contains the refactoring of the update code, the initialization of wine and the auto-symlinking of mounted volumes.

I've tested this in every way I can think possible, but would appreciate someone else performing some testing

Wine Initialization

  1. New container deploy - wine initializes
  2. Existing container start - wine initializing skipped

Symlinking

  1. Added volume - detected on next start of container and symlink created
  2. Removal of volume - nothing happens
  3. Change of drive letter of existing volume - symlink is created, but Backblaze ignores until the original drive letter is restored
  4. Replacing an existing drive letter with a new device - Backblaze offers to change the drive letter or replace the unplugged device - backblaze is unable to change the drive letter, but this is existing behavior if someone did this manually
  5. Adding 'replaced' device back as new drive letter - Backblaze picks up as new device now

Install/Updating

  1. New container with pinned version - installs
  2. Auto update with new pinned version - upgrades
  3. Forced update with no update available - advises no updates available
  4. Forced update with update available - updates

@JonathanTreffler
Copy link
Owner

JonathanTreffler commented Feb 7, 2024

Thanks @xela1 for your work. To me this seems to meet the quality requirements for a merge, but I will leave the final decision of that to @traktuner because he originally wrote the update code.

Also the new symlinking functionality requires the README to be changed: The unnecessary step(s) in the install process, that are now automated should be removed and new advisories on how to handle the drive letters, like you have already outlined above are needed. It would be nice of you to add these readme changes to the PR :)

@xela1
Copy link
Contributor Author

xela1 commented Feb 7, 2024

Also the new symlinking functionality requires the README to be changed: The unnecessary step(s) in the install process, that are now automated should be removed and new advisories on how to handle the drive letters, like you have already outlined above are needed. It would be nice of you to add these readme changes to the PR :)

Yes, I was thinking that earlier, I'll work on that when I finish work

@traktuner traktuner self-requested a review February 7, 2024 17:09
Copy link
Collaborator

@traktuner traktuner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ;)
Thank you for working on this and the improvements!

@traktuner traktuner merged commit 24455b0 into JonathanTreffler:main Feb 7, 2024
1 of 4 checks passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants