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

DietPi-Software | vaultwarden: Rename from Bitwarden_RS #4334

Merged
merged 17 commits into from
May 10, 2021
Merged

Conversation

Joulinar
Copy link
Collaborator

@Joulinar Joulinar commented May 3, 2021

Project has been renamed to avoid legal issues with the original Bitwarden software dani-garcia/vaultwarden#1642

Status: Testing

  • test vaultwarden install
  • test vaultwarden reinstall
  • test vaultwarden uninstall
  • test migration during dietpi-update

Reference: #4325

Commit list/description:

  • DietPi-Software | vaultwarden: project has been renamed to avoid legal issues with the original Bitwarden software
  • DietPi-Software | vaultwarden: use lower case on application name
  • DietPi-Software | vaultwarden: adjust reinstall to ensure transition from Bitwarden_RS to vaultwarden
  • DietPi-Services | vaultwarden: rename service from bitwarden_rs to vaultwarden DietPi-Patches | vaultwarden: add Bitwarden_RS migartion into vaultwarden
  • DietPi-Pre-Patches | vaultwarden: add detection for Bitwarden_RS and inform user about long run time
  • DietPi-Software | vaultwarden: remove Bitwarden_RS migration code as it has been moved into DietPi-Patches
  • DietPi-Patches | vaultwarden: show output of vaultwarden reinstall
  • DietPi-Patches | vaultwarden: add users notification
  • DietPi-Patches | vaultwarden: modify data directory setting inside vaultwarden.env
  • DietPi-Pre-Patches | vaultwarden: adjust user notification

DietPi-Software | Vaultwarden - project has been renamed to avoid legal issues with the original Bitwarden software
@Joulinar Joulinar marked this pull request as draft May 3, 2021 10:15
@Joulinar Joulinar added this to the v7.2 milestone May 3, 2021
@Joulinar Joulinar linked an issue May 3, 2021 that may be closed by this pull request
@Joulinar
Copy link
Collaborator Author

Joulinar commented May 3, 2021

@MichaIng
first attempt to reflect renaming from Bitwarden_RS into Vaultwarden. I tried to catch renaming on reinstall and uninstall. But I guess quite some testing needed. However I'm not sure where to get an old Bitwarden_RS installation to test on??

dietpi/dietpi-software Outdated Show resolved Hide resolved
dietpi/dietpi-software Outdated Show resolved Hide resolved
DietPi-Software | vaultwarden - use lower case on application name
DietPi-Software | vaultwarden - adjust reinstall to ensure transition from Bitwarden_RS to vaultwarden
@MichaIng
Copy link
Owner

MichaIng commented May 3, 2021

Hmhmhm. Another argument to force the reinstall is that the software name in the menu (uninstall, reinstall) and the docs changes as well. It might be confusing when one installed Bitwarden_RS and finally finds a vaultwarden install but no Bitwarden anymore and searching for Bitwarden in the docs does not show any result. At least we'd need to add a G_WHIP_MSG anyway, and then we can also say "sorry but due to this major upstream change we need to do a reinstall to apply it now. Grab yourself a coffee and you'll have the newest and safest password manager as reward for your patience!" or so 😄?

How long was it on RPi Zero? 1.5 hours if I remember right, or do I mix it up with another build?

@CactiChameleon9
Pinging you here, to allow participation. Any opinion whether we should force a reinstall to apply the renaming on next DietPi update or keep migration code and doubled uninstall code in dietpi-software to allow users deciding if/when to do it?

@MichaIng MichaIng changed the title DietPi-Software | Vaultwarden DietPi-Software | vaultwarden: Rename from Bitwarden_RS May 3, 2021
@CactiChameleon9
Copy link
Contributor

CactiChameleon9 commented May 3, 2021

I think you should force the reinstall. The main reason for me saying that is that dietpi-update is (to some extent) optional and completely manual (not automatic) process - therefore people should be aware that its happening and that these things can take a long time, as with all updates. This would also remove the extra effort of maintaining the migration code. Furthermore - you can notify people of the change (to prevent confusion) as you stated earlier.

BTW, I think you are correct in the 1.5 hour compile time on a pi zero (could be longer???)

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 3, 2021

Ok let's go for the migration right on dietpi-update as one of the first thinks, to avoid issues later on. This should simplify code in dietpi-software as well. Question: should we allow user to exit dietpi-update at this stage? Probably not everyone has that much time because usually updates are not that long running and people did not expect it.

@ravenclaw900
Copy link
Collaborator

Maybe make it a G_WHIP_YESNO, warning people about the change and asking them if they want to continue with the update?

@CactiChameleon9
Copy link
Contributor

CactiChameleon9 commented May 3, 2021

Yes, I like that idea. I think you should do the warning if bitwarden_rs was installed, and explain it has been renamed and so the update will take a while. Maybe an estimate too?? I assume that should be done before any changes were made, possibly point out to the user that its OK to cancel and update later if they want too

(raven beat me too it :D)

DietPi-Services | vaultwarden - rename service from bitwarden_rs to vaultwarden
@Joulinar
Copy link
Collaborator Author

Joulinar commented May 3, 2021

first clean installation on my RPi4B was working well. Compiling was done in around 30-35 minutes. One strange thing I observed. One the web page it is stating Version 2.19.0, which should be 2.21 if I'm not mistaken. During installation, correct archive was used:

[ OK ] DietPi-Software | Checking URL: https://github.com/dani-garcia/vaultwarden/archive/1.21.0.tar.gz

maybe the version tag was not updated? I placed a question on vaultwarden git.


EDIT
Ok issue was on my side. I misunderstood server and web-vault version

@CactiChameleon9
Copy link
Contributor

CactiChameleon9 commented May 3, 2021

One strange thing I observed. One the web page it is stating Version 2.19.0, which should be 2.21 if I'm not mistaken. During installation, correct archive was used:

@Joulinar
Isn't the 2.19.0 from the web vault version, from these lines in the script:

			local fallback_url='https://github.com/dani-garcia/bw_web_builds/releases/download/v2.19.0d/bw_web_v2.19.0d.tar.gz'
			Download_Install "$(curl -sSfL 'https://api.github.com/repos/dani-garcia/bw_web_builds/releases/latest' | mawk -F\" '/"browser_download_url": .*\/bw_web_[^"\/]*\.tar\.gz"/{print $4}')" /mnt/dietpi_userdata/vaultwarden

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 3, 2021

you are right @CactiChameleon9

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 5, 2021

@MichaIng
should this become part of pre-patch_file or patch_file?

@MichaIng
Copy link
Owner

MichaIng commented May 5, 2021

Okay, let's go with forced reinstall.

The reinstall/migration needs to be done in patches, as pre-patches are executed before any scripts have been updated, so a reinstall would then fail. pre-patches are mostly relevant for changes that must be done before APT upgrades, like APT repo, key or pinning changes, or for critical changes that might break APT upgrades in other ways. Scripts are updated after APT packages, so that we can count on latest package states for coding, and everything else, especially when it requires the new scripts in place, is done in patches.

For the G_WHIP_YESNO: We cannot allow to regularly exit from within patches, as then code version and migrations + /boot/dietpi/.version would not match. So the system would be half-updated, which might cause further errors. But we could add only the G_WHIP_YESNO to pre-patches, based on e.g. /mnt/dietpi_userdata/bitwarden_rs existence. Exiting there would be at a change where no update step would have been done, aside of mentioned APT key and repo updates, which are required in any case. exit 1 is respected by the parent dietpi-update script to error out then as well. The actual migration/reinstall is then done in patches.

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 5, 2021

ok I could try to get this moved into respective files if you are ok.

  • pre-patches will contain G_WHIP_YESNO allowing user to abort dietpi-update
  • patches will migrate folder structure and remove the service + user and trigger the reinstallation?

Would patches trigger dietpi-software reinstall directly or just set a trigger that will be catched up somewhere else? Something like echo 183 >> /var/tmp/dietpi/dietpi-update_reinstalls

@MichaIng
Copy link
Owner

MichaIng commented May 5, 2021

Would patches trigger dietpi-software reinstall directly

Jep. I didn't implement the old structure of patch_file into the new script yet. For now I think you can call dietpi-software directly. I'll add the structure to avoid doubled checks/patches and bundle reinstalls at the end of the script with a separate PR.

DietPi-Patches | add Bitwarden_RS migartion into vaultwarden
DietPi-Pre-Patches | add detection for Bitwarden_RS and inform user about long run time
DietPi-Software | remove Bitwarden_RS  migration code as it has been moved into DietPi-Patches
@Joulinar
Copy link
Collaborator Author

Joulinar commented May 8, 2021

Probably we should change wording inside user notification. Like 30 minutes up to several hours

@MichaIng
Copy link
Owner

MichaIng commented May 8, 2021

Makes sense. I mean users will probably remember best how long it took on their particular hardware when installing it the first time.

+ DietPi-Patches | vaultwarden migration: Reorder migration steps so that the data directory that is checked for whether to do the migration or not is renamed last, so that in case anything fails until the reinstall, it is repeated on next dietpi-update call.
+ DietPi-Patches | vaultwarden migration: Assure that no migration step can fail by checking for existing file/dir in every case, and error-handle everything. Assure that any renaming is done after all related edits are done, so that all related migration steps are repeated in case of failure.
+ DietPi-Patches | vaultwarden migration: Instead of error-handling the dietpi-software reinstall itself, exit with error code directly when it fails, as dietpi-software itself uses the error handler internally. So choosing "Exit" from within dietpi-software means then exiting dietpi-update as well.
+ DietPi-Pre-patches | vaultwarden migration: Make clear that the author has renamed the project
+ DietPi-Pre-patches | vaultwarden migration: Adjust possible max reinstall duration, which can take several hours, but the user will know best from the initial Bitwarden_RS install
@MichaIng
Copy link
Owner

MichaIng commented May 8, 2021

I reordered the migration steps a bit so that in case of any failure, steps which have not yet been done are assured to be repeated. Only when the dietpi-software reinstall call failed, it would not be done, but it's probably best to call dietpi-software directly then.

Ahh, actually it makes sense to allow the reinstall itself to error out and not exit the updater in this case. As all migration steps have been done, dietpi-update had no task left, so its okay to allow it finishing and print info to the user to reinstall the new vaultwarden option instead (and skip the updater wrapper/overhead by this). This also allows users to CTRL+C interrupt the reinstall and return to it later, of course with a nun-functional Bitwarden_RS/vaultwarden then.

@MichaIng
Copy link
Owner

MichaIng commented May 8, 2021

Jep, dietpi-software output needs to be there, but it is as it is not wrapped into G_EXEC now. This does not make much sense as it uses G_EXEC internally. In case of failure, when user cannot fix the issue by repeating or such and hits Exit, (s)he would land in a second error handler prompt then, which is a bit strange? But as said above, I think we should allow the final reinstall to fail.

... jep I think that is fine. Would be bad to abort the whole update when everything else has been successfully finished. So users can choose when they find time to repeat the lengthy reinstall.

+ DietPi-Patches | Allow vaultwarden reinstall to fail. It is okay to allow the user doing this manually at a later time without aborting the whole dietpi-update, as all required migration steps have been done already.
+ DietPi-Pre-patches | Remove doubled during
@MichaIng
Copy link
Owner

MichaIng commented May 8, 2021

I removed the second "during", it is still a correct sentence 😅?

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 8, 2021

I guess users are more happy with good coding instead of grammar 🤣

@MichaIng
Copy link
Owner

MichaIng commented May 8, 2021

Shall we add an info that all data and configs will be preserved?

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 8, 2021

That's actually a good idea

+ DietPi-Pre-patches | vaultwarden migration: Add an info that data and configs will be preserved to prevent questions about that pro-actively
.update/pre-patches Outdated Show resolved Hide resolved
+ DietPi-Pre-patches | Spelling
@MichaIng
Copy link
Owner

MichaIng commented May 8, 2021

[FAILED] DietPi-Software | Automatic latest vaultwarden version detection failed. Version "1.21.0" will be installed as fallback, but a newer version might be available. Please report this at: https://github.com/MichaIng/DietPi/issues

Strange, when running the command to get the version manually, it works fine:

version=$(curl -sSfL 'https://api.github.com/repos/dani-garcia/vaultwarden/releases/latest' | mawk -F\" '/"tag_name": /{print $4}')
[[ $version ]] || echo failed

Temporary connection issue? Will do reinstall anyway, but please check whether you see this error as well when testing.

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 9, 2021

At least during my test it was wondering. Will do another one

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 9, 2021

I did 2 test now. Both are fine from my side

 DietPi-Software
─────────────────────────────────────────────────────
 Mode: Installing vaultwarden: Unofficial Bitwarden password manager server written in Rust

[  OK  ] DietPi-Software | Checking URL: https://github.com/dani-garcia/vaultwarden/archive/1.21.0.tar.gz
[  OK  ] DietPi-Software | cd /tmp/DietPi-Software
[ INFO ] DietPi-Software | G_THREAD_START_0 | curl -sSfL https://github.com/dani-garcia/vaultwarden/archive/1.21.0.tar.gz -o 1.21.0.tar.gz
[ INFO ] DietPi-Software | APT install for: pkg-config libssl-dev, please wait...
[  OK  ] DietPi-Software | APT install for: pkg-config libssl-dev
[  OK  ] DietPi-Software | G_THREAD: All threads finished
[  OK  ] DietPi-Software | tar xf 1.21.0.tar.gz
[  OK  ] DietPi-Software | rm 1.21.0.tar.gz
 DietPi-Software
─────────────────────────────────────────────────────
 Mode: Installing vaultwarden: Unofficial Bitwarden password manager server written in Rust

[  OK  ] DietPi-Software | Checking URL: https://github.com/dani-garcia/vaultwarden/archive/1.21.0.tar.gz
[  OK  ] DietPi-Software | cd /tmp/DietPi-Software
[ INFO ] DietPi-Software | G_THREAD_START_0 | curl -sSfL https://github.com/dani-garcia/vaultwarden/archive/1.21.0.tar.gz -o 1.21.0.tar.gz
[ INFO ] DietPi-Software | APT install for: pkg-config libssl-dev, please wait...
[  OK  ] DietPi-Software | APT install for: pkg-config libssl-dev
[  OK  ] DietPi-Software | G_THREAD: All threads finished
[  OK  ] DietPi-Software | tar xf 1.21.0.tar.gz
[  OK  ] DietPi-Software | rm 1.21.0.tar.gz
[  OK  ] DietPi-Software | rm -R /opt/vaultwarden
[  OK  ] DietPi-Software | mv vaultwarden-1.21.0 /opt/vaultwarden

@MichaIng
Copy link
Owner

MichaIng commented May 9, 2021

Strange is that I also didn't see any curl error message (which is always shown, including HTTP error codes), like the connection was fine, but the content not as expected. However, seems to have been some temporary issue then, second run in my case went fine as well.

One thing we need to change is the amount of required memory, at least when more than one core is available. With VMs I ran into compile failures as the memory was full. They have 2 GB RAM (which makes the installer use two of four available cores) and a little higher idle usage (as e.g. APT cache + lists are in RAM) but it seems to be quite close and it would be a nightmare if you wait for hours on an RPi Zero and it fails in the middle with unspecified error message (it does not say something about OOM in a word) and even retries to compile the same thing with the other cores (when available) for a while which is doomed to fail. We could also limit the build jobs/core to 1, but that would ~double the compile time.

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 9, 2021

Probably better to increase swap instead. On my RPi3B+ I added 4GB swap to enable compiling on all 4 core.

@Joulinar
Copy link
Collaborator Author

Joulinar commented May 9, 2021

reinstall + uninstall was working for me during my test.

@MichaIng
Copy link
Owner

MichaIng commented May 9, 2021

Jep works all fine here as well. I'll merge it. Memory usage will be addressed in a separate PR as probably this requires an extension for dietpi-set_swapfile to e.g. allow auto-applying a swap size so that 2.5 GiB overall memory is available. Currently only for 2 GiB overall memory, the swap size can be auto-applied.

@MichaIng MichaIng merged commit b82dfe7 into dev May 10, 2021
@MichaIng MichaIng deleted the vaultwarden branch May 10, 2021 12:53
@MichaIng
Copy link
Owner

Changelog: e32e44f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DietPi-Software | Bitwarden_RS has been renamed to vaultwarden
4 participants