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

feat: allow to configure the apt upgrade method on debian #749

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thvitt
Copy link

@thvitt thvitt commented Mar 23, 2024

dist-upgrade, which was the hard-coded option, is allowed to remove packages in order to resolve dependency conflicts. This might cause quite a mess ... it is left as the default, but can be modified by the option apt_command.

Standards checklist:

  • The PR title is descriptive.
  • I have read CONTRIBUTING.md
  • The code compiles (cargo build)
  • The code passes rustfmt (cargo fmt)
  • The code passes clippy (cargo clippy)
  • The code passes tests (cargo test)
  • Optional: I have tested the code myself

For new steps

  • Optional: Topgrade skips this step where needed
  • Optional: The --dry-run option works with this step
  • Optional: The --yes option works with this step if it is supported by
    the underlying command

If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.

Copy link

codecov bot commented Mar 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 4.98%. Comparing base (373cd3b) to head (eed298c).

Files Patch % Lines
src/config.rs 0.00% 9 Missing ⚠️
src/steps/os/linux.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #749      +/-   ##
========================================
- Coverage   5.03%   4.98%   -0.06%     
========================================
  Files         37      37              
  Lines      12085   12216     +131     
========================================
  Hits         609     609              
- Misses     11476   11607     +131     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SteveLauC
Copy link
Member

Hi, thanks for your interest in contributing to Topgrade!

The CI failures are not related, please rebase your branch (against main) to get them resolved:)

dist-upgrade, which was the hard-coded option, is allowed to remove packages in order to resolve dependency conflicts. This might cause quite a mess

I can see this disaster, in your opinion, what is the appropriate default command to use? And, I am thinking, it is suitable for most users, then we can actually replace dist-upgrade with it

dist-upgrade, which was the hard-coded option, is allowed to remove
packages in order to resolve dependency conflicts. This might cause
quite a mess ... it is left as the default, but can be modified by the
option apt_command.
@thvitt
Copy link
Author

thvitt commented Mar 24, 2024

I can see this disaster, in your opinion, what is the appropriate default command to use? And, I am thinking, it is suitable for most users, then we can actually replace dist-upgrade with it

upgrade certainly is the safer variant, OTOH I am using topgrade --yes on Debian testing, and they are just migrating to a 64bit datetime type, so I shouldn’t be too surprised :)

My use case for topgrade is 'keep the system relatively up-to-date without too much hassle', so for that, upgrade would be the better default. BTW it looks like it’s the default for nala anyway (nala’s equivalent to apt-get dist-upgrade is nala full-upgrade)

@SteveLauC
Copy link
Member

BTW it looks like it’s the default for nala anyway (nala’s equivalent to apt-get dist-upgrade is nala full-upgrade)

Thanks for the info!


I am thinking about:

  1. use apt upgrade as the default option (for apt-fast and apt-get)

  2. Make the subcommand for nala and apt-fast/apt-get configurable

    available options that can be specified in the config file:

    1. upgrade (can be used with nala/apt-fast/apt-get)
    2. dist-upgrade (ONLY available for apt-fast/apt-get)
    3. full-upgrade (ONLY available for nala)
  3. If an invalid option is specified in the config file, print an error message and skip the step during step execution (i.e., we don't do check if the config is valid while loading configuration)

  4. For the config entry name, we might want to use something like this, specifying:

    1. It is for Debian-like distros
    2. This is the "upgrade" sub-command
    # The upgrade subcommand to use on Debian-like distros
    # For `apt-get` and `apt-fast`, one can use:
    # 1. "upgrade"
    # 2. "dist-upgrade"
    #
    # For `nala`, available options:
    # 1. "upgrade"
    # 2. "full-upgrade"
    debian_like_apt_upgrade_cmd = "value"

I would like to hear your thoughts on this since I am not a Debian user:)

@thvitt
Copy link
Author

thvitt commented Apr 2, 2024

The longer I think about it:

  • I agree that upgrade should be the default as safest option.
  • nala full-upgradeapt-get dist-upgrade --autoremove
  • You can, in theory, have apt, apt-fast, and nala installed in parallel and use a different one of them each day, since they all work on the same package store.

I think it is a little confusing to configure the subcommand by hand but let the package manager be autodetected. So maybe the most sensible variant is to have the auto-detected tool with upgrade as default and just offer one configuration option that can be set to the whole command line, like debian_like_upgrade_cmdline = "apt-fast full-upgrade --autoremove"? The update command could just take the first word of this and run it with update, since that is all the same.

Alternatively, maybe a boolean option debian_like_dist_upgrade that, when true, selects dist-upgrade or full-upgrade depending on the detected package manager.

@SteveLauC
Copy link
Member

SteveLauC commented Apr 13, 2024

I think it is a little confusing to configure the subcommand by hand but let the package manager be autodetected. So maybe the most sensible variant is to have the auto-detected tool with upgrade as default and just offer one configuration option that can be set to the whole command line,

Yeah, I agree that it feels weird

like debian_like_upgrade_cmdline = "apt-fast full-upgrade --autoremove"?

Though I feel Topgrade should not do such things (let the user write the whole script) either

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.

2 participants