refactor: update package lists only when stale #346

2023-06-03T23:47:45+02:00

cravl-dev commented

2023-06-03 23:47:45 +02:00

(Migrated from github.com)

Fixes #334 by first checking if any lists are more than 3 days old. Should be foolproof—if the check returns an error (e.g. if the find command or the /var/lib/apt/lists directory doesn't exist), it defaults to running the update.

Fixes #334 by first checking if any lists are more than 3 days old. Should be foolproof—if the check returns an error (e.g. if the `find` command or the `/var/lib/apt/lists` directory doesn't exist), it defaults to running the update.

dw-0 commented

2023-06-04 16:45:37 +02:00

(Migrated from github.com)

Thanks for the PR, i really like the idea as it saves some time and removes redundant update calls.
Maybe we want to use a dedicated function for that now? So we can get rid of duplicated code. maybe https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh would be a proper file to place that function in imho.

Thanks for the PR, i really like the idea as it saves some time and removes redundant update calls. Maybe we want to use a dedicated function for that now? So we can get rid of duplicated code. maybe https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh would be a proper file to place that function in imho.

cravl-dev commented

2023-06-08 06:48:33 +02:00

(Migrated from github.com)

Alright, that's 1000% better! Change summary is as follows:

Split out update_system_package_lists as its own function, now uses coreutils' stat instead of find to check the cache age which should be universally supported. Of course it still defaults to running the update if the check somehow fails. As an added bonus it also has an optional --silent flag, used currently to prevent rogue apt output from screwing with the update menu.
Created a new install_system_packages function to further de-duplicate even more code across all the module install functions. Takes as arguments $1) a name to put into the log line, and $2) the name of the array of packages to install (using this fantastically sneaky trick).

Alright, that's 1000% better! Change summary is as follows: - Split out `update_system_package_lists` as its own function, now uses coreutils' `stat` instead of `find` to check the cache age which should be universally supported. Of course it still defaults to running the update if the check somehow fails. As an added bonus it also has an optional `--silent` flag, used currently to prevent rogue apt output from screwing with the update menu. - Created a new `install_system_packages` function to further de-duplicate even more code across all the module install functions. Takes as arguments $1) a name to put into the log line, and $2) _the name of_ the array of packages to install (_using this [fantastically sneaky trick](https://stackoverflow.com/a/4017175)_).

cravl-dev commented

2023-06-08 17:08:24 +02:00

(Migrated from github.com)

style: rebased to conform to existing semantic commit message style

_style: rebased to conform to existing semantic commit message style_

dw-0 commented

2023-06-10 10:15:28 +02:00

(Migrated from github.com)

Hey, thanks for the PR, i really like it! I added some missing curly braces around variables and refactored the update_system_package_lists function a bit.

There is still a case in regards to the check_system_updates function. There is a if condition which checks if the command failed or not in line 396.
If the update age is too old, we would do a sudo apt-get update in line 381. There is the first thing i don't quite like.
If entering the update menu, this will cause a password prompt while rendering the update status window once the system check is reached. Interrupting the rendering of the window. I currently don't have a good idea for a fix though, as the sudo is usually required to not get permission issues.

Then the second thing i spotted. In case sudo apt-get update --allow-releaseinfo-change fails, line 384 would exit the full script. Although that it probably fine during the installs of Klipper, Moonraker or any other component we use that function for now, it likely is not desired in case of the update menu as it currently would just exit the script without any feedback, right?
I saw that you would switch the status in the update window for the system to Update check failed! in line 397, but that line would never be reached due to the exit, or?

Hey, thanks for the PR, i really like it! I added some missing curly braces around variables and refactored the `update_system_package_lists` function a bit. There is still a case in regards to the check_system_updates function. There is a if condition which checks if the command failed or not in [line 396](https://github.com/th33xitus/kiauh/pull/346/commits/9b64199a2fa79e193b43f6a8d2bb51ad8ef72722#diff-51c13f857871ab2262217665d19263f641f7765a3047b917380c56e6271a0c0bR396). If the update age is too old, we would do a sudo apt-get update [in line 381](https://github.com/th33xitus/kiauh/pull/346/commits/9b64199a2fa79e193b43f6a8d2bb51ad8ef72722#diff-51c13f857871ab2262217665d19263f641f7765a3047b917380c56e6271a0c0bR381). There is the first thing i don't quite like. If entering the update menu, this will cause a password prompt while rendering the update status window once the system check is reached. Interrupting the rendering of the window. I currently don't have a good idea for a fix though, as the sudo is usually required to not get permission issues. Then the second thing i spotted. In case `sudo apt-get update --allow-releaseinfo-change` fails, line 384 would exit the full script. Although that it probably fine during the installs of Klipper, Moonraker or any other component we use that function for now, it likely is not desired in case of the update menu as it currently would just exit the script without any feedback, right? I saw that you would switch the status in the update window for the system to `Update check failed!` [in line 397](https://github.com/th33xitus/kiauh/pull/346/commits/9b64199a2fa79e193b43f6a8d2bb51ad8ef72722#diff-51c13f857871ab2262217665d19263f641f7765a3047b917380c56e6271a0c0bR397), but that line would never be reached due to the exit, or?

cravl-dev commented

2023-06-12 05:50:37 +02:00

(Migrated from github.com)

Thanks for the review! I think I've gotten both of those items resolved now.

The update + install menu scripts now refresh the cached credentials with sudo -v (which prompts for password only if needed) and then clear the screen (but not the scrollback) before drawing, so that the menu will draw clean every time. Install menu was mostly just for consistency but could be helpful for long-running install sessions to prevent hitting the 15 minute timeout.

Then, the update_system_package_lists function now simply does a return 1 on fail rather than exit 1. I think this is what I intended originally but I got the flow a bit mixed up in my head. If you want to, it's easy to ensure package installation remains dependent on a successful update:

if update_system_package_lists; then
  install_system_packages "${log_name}" "packages[@]"
else
  error_msg "Aborting package installation" # Error for failed update will have printed directly above
fi

Thanks for the review! I think I've gotten both of those items resolved now. The update + install menu scripts now refresh the cached credentials with `sudo -v` (which prompts for password only if needed) and then clear the screen (but not the scrollback) before drawing, so that the menu will draw clean every time. Install menu was mostly just for consistency but could be helpful for long-running install sessions to prevent hitting the 15 minute timeout. Then, the `update_system_package_lists` function now simply does a `return 1` on fail rather than `exit 1`. I think this is what I intended originally but I got the flow a bit mixed up in my head. If you want to, it's easy to ensure package installation remains dependent on a successful update: ```sh if update_system_package_lists; then install_system_packages "${log_name}" "packages[@]" else error_msg "Aborting package installation" # Error for failed update will have printed directly above fi ```

dw-0 commented

2023-06-17 18:08:08 +02:00

(Migrated from github.com)

Thanks! I think we can do it like that. The user has to put in the password anyways if something needs to be installed or maybe updated. So i think its fine to ask for the password directly before even printing the UI.

I don't think we need to restrict package installation to a successful package list update, unless there will be reports in the future, that a situation like this causes problems.

Thank you! Im merging this PR as is for now :)

Thanks! I think we can do it like that. The user has to put in the password anyways if something needs to be installed or maybe updated. So i think its fine to ask for the password directly before even printing the UI. I don't think we need to restrict package installation to a successful package list update, unless there will be reports in the future, that a situation like this causes problems. Thank you! Im merging this PR as is for now :)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: h3n3/kiauh#346