Centralize all standard LXC options to avoid code duplication

[remz1337]

Note

We are meticulous when it comes to merging code into the main branch, so please understand that we may reject pull requests that do not meet the project's standards. It's never personal. Also, game-related scripts have a lower chance of being merged.

Description

Add a function, base_settings, to centralize all common options so that scripts only need to define settings that need to be overridden. For the great majority of scripts, all the options use the default values for default_settings. Key settings are already captured outside of the default_settings function: var_XXX (eg. var_disk, var_cpu ...)

With this PR, the default_settings function in scripts could go from:

function default_settings() {
  CT_TYPE="1"
  PW=""
  CT_ID=$NEXTID
  HN=$NSAPP
  DISK_SIZE="$var_disk"
  CORE_COUNT="$var_cpu"
  RAM_SIZE="$var_ram"
  BRG="vmbr0"
  NET="dhcp"
  GATE=""
  APT_CACHER=""
  APT_CACHER_IP=""
  DISABLEIP6="no"
  MTU=""
  SD=""
  NS=""
  MAC=""
  VLAN=""
  SSH="no"
  VERB="no"
  echo_default
}

to

function default_settings() {
  CT_ID=$NEXTID
  HN=$NSAPP
  DISK_SIZE="$var_disk"
  CORE_COUNT="$var_cpu"
  RAM_SIZE="$var_ram"
  echo_default
}

and in case a script needs to overwrite an additional setting, simply add it back, like this:

function default_settings() {
  CT_ID=$NEXTID
  HN=$NSAPP
  DISK_SIZE="$var_disk"
  CORE_COUNT="$var_cpu"
  RAM_SIZE="$var_ram"
  DISABLEIP6="yes"
  VERB="yes"
  echo_default
}

I've been using this in my fork for some time. It's great when you need to add a new default variable, you don't need to update back all the scripts. Plus, it's good practice to avoid duplication of information.

Type of change

Please check the relevant option(s):

• Bug fix (non-breaking change that resolves an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (a fix or feature that would cause existing functionality to change unexpectedly)
• New script (a fully functional and thoroughly tested script or set of scripts.)

Prerequisites

The following efforts must be made for the PR to be considered. Please check when completed:

• Self-review performed (I have reviewed my code, ensuring it follows established patterns and conventions)
• Testing performed (I have tested my changes, ensuring everything works as expected)
• Documentation updated (I have updated any relevant documentation)

[remz1337]

If this get merged, I will do a second PR to go back and cleanup all the default_settings function in every script

[newzealandpaul]

I think this is a good idea.

[newzealandpaul]

@remz1337 I don't think a bulk script update should be done, I think it would be better to merge this, and script authors update scripts as we go, ensuring there are no side effects.

[MickLesk]

Standard HDD size 4GB is a bit too much I think.we have some scripts with less.

How do you set an LXC to Privileged?

What about the alpine scripts?

How do you set the script to verbose? It must be understandable for every beginner.

Many questions... 😅

[havardthom]

We should also consider that this will make it harder for contributors to know what settings exist, though everyone probably just copies it today without thinking about it anyway

[newzealandpaul]

I think this makes sense, we can use documentation to ensure users are able to understand what options can be overridden.

[newzealandpaul]

I approved @havardthom changes to wording. We need one more approval for this to be merged. I think it makes a lot of sense and I will volunteer to write some documentation.

[newzealandpaul]

@MickLesk checking in with you on this. This is a major change so I think we need consensus rather than just two approvals.

[MickLesk]

its wrong here? Need to replace with Row 529 ?

[remz1337]

no, essentially you load all the standard (base) settings (eg. 4GB HDD, 512Mb RAM, 1 CPU...) with the base_settings function, then calling default_settings allows to overide them with whatever is defined in the actual installation script (eg. 20Gb HDD, 4CPU...)

[MickLesk]

It must be possible to overwrite the following functions by default. (From the LXC Install Script):
Maybe I'm thinking too complex, but I don't think the current solution really helps. Or am I wrong? Here is my idea for a solution:

CT_TYPE="$var_unprivileged"
DISK_SIZE="$var_disk”
CORE_COUNT="$var_cpu”
RAM_SIZE="$var_ram”

As Example for this
Komga - Now:

  function header_info {
clear
cat <<"EOF"
    __ __                           
   / //_/___  ____ ___  ____ _____ _
  / ,< / __ \/ __ `__ \/ __ `/ __ `/
 / /| / /_/ / / / / / / /_/ / /_/ / 
/_/ |_\____/_/ /_/ /_/\__, /\__,_/  
                     /____/           
EOF
}
header_info
echo -e "Loading..."
APP="Komga"
var_disk="4"
var_cpu="1"
var_ram="2048"
var_os="debian"
var_version="12"
variables
color
catch_errors

function default_settings() {
  CT_TYPE="1"
  PW=""
  CT_ID=$NEXTID
  HN=$NSAPP
  DISK_SIZE="$var_disk"
  CORE_COUNT="$var_cpu"
  RAM_SIZE="$var_ram"
  BRG="vmbr0"
  NET="dhcp"
  GATE=""
  APT_CACHER=""
  APT_CACHER_IP=""
  DISABLEIP6="no"
  MTU=""
  SD=""
  NS=""
  MAC=""
  VLAN=""
  SSH="no"
  VERB="no"
  echo_default
}

function update_script() {
header_info
check_container_storage
check_container_resources

vs. Komga new:

function header_info {
clear
cat <<"EOF"
    __ __                           
   / //_/___  ____ ___  ____ _____ _
  / ,< / __ \/ __ `__ \/ __ `/ __ `/
 / /| / /_/ / / / / / / /_/ / /_/ / 
/_/ |_\____/_/ /_/ /_/\__, /\__,_/  
                     /____/           
EOF
}
header_info
echo -e "Loading..."
APP="Komga"
var_unprivileged="1"
var_disk="4"
var_cpu="1"
var_ram="2048"
var_os="debian"
var_version="12"
variables
color
catch_errors
base_settings

function update_script() {
  header_info
  check_container_storage
  check_container_resources

and in build.func the base_settings:

base_settings() {
  # Default Settings
  CT_TYPE="1"
  DISK_SIZE="4"
  CORE_COUNT="1"
  RAM_SIZE="1024"
  
  # Changed From CT.sh 
  CT_TYPE=${var_unprivileged:-$CT_TYPE}
  DISK_SIZE=${var_disk:-$DISK_SIZE}
  CORE_COUNT=${var_cpu:-$CORE_COUNT}
  RAM_SIZE=${var_ram:-$RAM_SIZE}
  
  # Other Default Settings (unchangeble) 
  PW=""
  CT_ID=$NEXTID
  HN=$NSAPP
  BRG="vmbr0"
  NET="dhcp"
  GATE=""
  APT_CACHER=""
  APT_CACHER_IP=""
  DISABLEIP6="no"
  MTU=""
  SD=""
  NS=""
  MAC=""
  VLAN=""
  SSH="no"
  VERB="no"
  # Since these 2 are only defined outside of default_settings function, we add a temporary fallback. TODO: To align everything, we should add these as constant variables (e.g. OSTYPE and OSVERSION), but that would currently require updating the default_settings function for all existing scripts
  if [ -z "$var_os" ]; then
    var_os="debian"
  fi
  if [ -z "$var_version" ]; then
    var_version="12"
  fi
}

[Mellowlynx]

Not a fan.
Keep it simple and have all the used specs in the script itself.
This will cause more testing errors for unexperienced contributions, as that will not know or forget to update something.

Having it right there, makes it simple for everyone to know, what is up.

[remz1337]

Hey, catching up on this. Thanks for your comments @MickLesk

Standard HDD size 4GB is a bit too much I think.we have some scripts with less.

Yes maybe we can set it it 2 or 3 as default, I don't mind. For me, default should be enough to spin up an "empty" container (eg. debian, without anything else)

How do you set an LXC to Privileged?

this doesn't change, "CT_TYPE=0"

What about the alpine scripts?

Not sure about this, I will have to check.

How do you set the script to verbose? It must be understandable for every beginner.

same here, "VERB=yes"

Many questions... 😅

Let me know if you have any other questions

And regarding your other comment (I won't be quoting everything, but the one that start with):

It must be possible to overwrite the following functions by default. (From the LXC Install Script):
....

If you do it that way, then it's not possible anymore for the scripts to override other settings such as VLAN, VERB... The goal of this PR is more about removing the need for every script to redefine things that are always standard (like VERB=no)

[remz1337]

@havardthom regarding your comment

We should also consider that this will make it harder for contributors to know what settings exist, though everyone probably just copies it today without thinking about it anyway

We could create some documentation (either MD file in the repo or a page in the wiki) with all the possible options + values. And maybe what we do is keep some common settings commented in there (so they only have to uncomment is they want to override them)?

[MickLesk]

My comments above was old. I think you should look at the last idea I had, I think that can or should be the only right solution..😅
This should cover all use cases for default Debian and Ubuntu.

[remz1337]

My comments above was old. I think you should look at the last idea I had, I think that can or should be the only right solution..😅 This should cover all use cases for default Debian and Ubuntu.

You mean the example you gave with Komga? With your suggestion, scripts can't override some defaults if needed (like changing DISABLEIP6="no" to DISABLEIP6="yes". With my suggestion, you override it by including it in the default_settings function.

[remz1337]

I checked some alpine scripts. My PR would work with them too, no issues

[MickLesk]

Ive build an complete rework of this logic:

Default Settings

Advanced Settings (i know cores Icon missing in this screenshot)

Here is the git change in build.func:
https://github.com/MickLesk/Proxmox_DEV/blob/a2b79b93393f3c63ba409aa945522230c477ea1f/misc/build.func#L158-L218

Here is the git change of nextpvr.sh (CT):
https://github.com/MickLesk/Proxmox_DEV/blob/50f41bf518620aa5bd2623cf5c9d81bafa2bfdfe/ct/nextpvr.sh#L9-L25

Feel free to test all variables with:

 bash -c "$(wget -qLO - https://raw.githubusercontent.com/MickLesk/Proxmox_DEV/refs/heads/main/ct/nextpvr.sh)"

[oOStroudyOo]

Who defines the default settings?
Is it me (the user), or the developer of the script?

As a user of the scripts, I do find it repetitive to need to set the same things over and over again. I'd like to be able to set my own preferred settings.

My personally I always select Debian 12.
I have my IP address set so it ends in the same number as the container number (i.e. CT = 108, IP address = 192.168.0.108)
I have the same gateway IP every time.
The same Apt-cacher IP every time.
I have a specific LXC password.
and so on..

This would be really cool for the user to define.
There could be a script that the user can run in pve shell that goes through each option where you define or remove a default option.
I believe there is a plex-update script does something similar to this, and you can adjust the settings by running the plex-update script in the plex container again to define when the updater tool runs and if it should update if the server is currently in use and etc.

So in our case, we run a Proxmox Helper Script Defaults script in PVE shell and we go through each option:
Would you like to always use the next available CT number? yes or no?
Would you like the code owner to define the CT name? yes or no?
Would you like to set a default apt-cache IP? yes, no, not installed <-- for those that don't use apt-cacher to silence that prompt.

[MickLesk]

This exceeds the scope and would be possible (if it can be migrated without any problems at all) sometime in mid-2025. So many things need to be changed that you cannot imagine. The fact that the scripts probably run on more than 25,000 Proxmox servers must also be taken into consideration.

We want to keep it clean, simple and easy to understand.