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

Add wrapper for package manipulation #320

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add wrapper for package manipulation #320

wants to merge 15 commits into from

Conversation

dimitry-ishenko
Copy link
Collaborator

@dimitry-ishenko dimitry-ishenko commented Dec 12, 2024

What do you guys think about abstracting away package installation and removal?

This will help add consistency and reduce boilerplate. Usage is as follows:

package install vim
package remove mc
package is-installed nmap
etc.

This will also replace check_if_installed and apt_install_wrapper commands.

@github-actions github-actions bot added the size/small PR with less then 50 lines label Dec 12, 2024
@Tearran
Copy link
Member

Tearran commented Dec 12, 2024

@dimitry-ishenko This could be use to consolidate and refine or replace some of the following. Your idea is a good.

function check_if_installed() {

function apt_install_wrapper() {

Reference for helper function naming standards,
I suggest

["module_packages,example"]="help install remove update upgrade check"

or

["set_packages,example"]="help install remove update upgrade check"

note
IMO, Seems the inconsistency more a matter of a defining a procedure to follow then automating and document it. This is difficult while the remains database is incomplete, Part of the database is json and part associative array . The module_option array is missing data points that exist in the json. That need to be added to all the module_options.

@dimitry-ishenko
Copy link
Collaborator Author

Thanks @Tearran. Are you suggesting to rename the file (ie, package.sh -> module_packages.sh) or also change the name of the function? I chose the name package because it makes the code natural and easy to read.

By the standard it would have to be something like:

get_package vim
rem_package mc
see_package_installed nmap

IMHO these are a bit less natural, but if that's what we gotta do, then that's what we gotta do.

@Tearran
Copy link
Member

Tearran commented Dec 12, 2024

Just the function name the prefix is used to separate groups. So if this is a module use module_ if it's a helper to be used with a module see_ set_ even etc..

This I would consider both so your call but only one prefix name.

We can even refine and define new prefix to clarify things. We do have "generate_" , Perhaps, "helper_" "manage_".

@dimitry-ishenko
Copy link
Collaborator Author

dimitry-ishenko commented Dec 12, 2024

Just the function name the prefix is used to separate groups. So if this is a module use module_ if it's a helper to be used with a module see_ set_ even etc..

This I would consider both so your call but only one prefix name.

We can even refine and define new prefix to clarify things. We do have "generate_" , Perhaps, "helper_" "manage_".

TBH none of the prefixes really fit here... How about adding a package_ or pkg_ prefix? Then we would have:

package_install foo
package_remove bar
package_is_installed baz

or

pkg_install foo
pkg_remove bar
pkg_is_installed baz

Please see 👇🏻

@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines and removed size/small PR with less then 50 lines labels Dec 12, 2024
@github-actions github-actions bot added size/small PR with less then 50 lines and removed size/medium PR with more then 50 and less then 250 lines labels Dec 12, 2024
@Tearran
Copy link
Member

Tearran commented Dec 13, 2024

package_ should work well. although IMO pkg_ fits the 3 letter prefix better, but that is unimportant ;)

would also suggest simply "check" or "status" for the is_installed.

package_install foo
package_remove bar
package_check baz

Quick fix : "Disabled" or ""

	["package_<job name>,status"]=""

If you don't mind helping refine module_option.
The module_option status, may be misleading as it is. Last check "Disabled" is the only string that is used.

Reference:

filter_disabled() {

&&
done < <(echo "$json_data" | jq -r '.menu[] | '${parent_id:+".. | objects | select(.id==\"$parent_id\") | .sub[]? |"}' select(.status != "Disabled") | "\(.id)\n\(.description)\n\(.condition)"' || exit 1)

I think we can leverage this to define what the function is.

	["package_<job name>,status"]="Helper"

For modules it would be. System, Network, Localization, Software. and any other category that may be needed at later date.
#321 is about finishing the database then replace some slow bash parsing with JQ (json query).

@dimitry-ishenko
Copy link
Collaborator Author

package_ should work well. although IMO pkg_ fits the 3 letter prefix better, but that is unimportant ;)

Great idea, I will change it to pkg_.

@dimitry-ishenko
Copy link
Collaborator Author

If you don't mind helping refine module_option.
The module_option status, may be misleading as it is. Last check "Disabled" is the only string that is used.

I am still getting a feel for what does what, where and how 😃 but I can take a look.

@dimitry-ishenko
Copy link
Collaborator Author

@Tearran should I keep the name of the file as package.sh?

@Tearran
Copy link
Member

Tearran commented Dec 13, 2024

no standard set, as for the logic it is inconsequential, so snakecase "any_name_will_do.sh"

function merge_modules(){

Software does set a president as it uses the prefix "install_"
If we want to follow this naming consistency IMO "helper_" would be appropriate for this new and existing helpers in functions folder for, naming conventions

@Tearran
Copy link
Member

Tearran commented Dec 13, 2024

@dimitry-ishenko in short code wise currently it does not matter can leave it if you like. But :) this is a good place to clarify, and refine existing precedent for consistency.

tested all works with --api flag.
Seems like Non interactive UX so perhaps should be "interface_" for file prefix.

./bin/armbian-config --api package_install whiptail
{EB4B9D6B-308F-40A0-9631-9FB3C94A0365}

precedent

https://github.com/armbian/configng/blob/main/tools/modules/functions/interface_menu.sh

Note : The [[ -t 0 ]] Does Not work with the --api option

@dimitry-ishenko dimitry-ishenko force-pushed the package branch 2 times, most recently from 7188227 to 45b2e10 Compare December 13, 2024 16:02
@dimitry-ishenko dimitry-ishenko marked this pull request as ready for review December 13, 2024 16:07
@dimitry-ishenko dimitry-ishenko force-pushed the package branch 2 times, most recently from a4087db to c697dc6 Compare December 13, 2024 16:51
@dimitry-ishenko
Copy link
Collaborator Author

dimitry-ishenko commented Dec 13, 2024

This should be ready to go. Works in non-interactive mode as well.

I've opted for pkg_installed instead of pkg_check or pkg_status because "check" or "status" can mean many different things. For example, this reads like almost natural English:

if pkg_installed zfsutils-linux; then
    ...
fi

@dimitry-ishenko
Copy link
Collaborator Author

dimitry-ishenko commented Dec 13, 2024

Note : The [[ -t 0 ]] Does Not work with the --api option

@Tearran currently there is no differentiation between the menu and the --api option. One thing we can do is close stdin when using --api and it will run without the UI:

"--api")
	shift
	if [[ -z "$1" || "$1" == "help" ]]; then
		see_use
		exit 0
	fi
	option="$1" 	shift
 	args=$(sanitize_input "$@")
 	# echo -e "\"$option\" \"$args\""
-	"$option" "$args"
+	"$option" "$args" <&-
 	exit 0
 	;;

@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines and removed size/small PR with less then 50 lines labels Dec 13, 2024
@github-actions github-actions bot added size/large PR with 250 lines or more and removed size/medium PR with more then 50 and less then 250 lines labels Dec 14, 2024
@Tearran
Copy link
Member

Tearran commented Dec 15, 2024

@igorpecovnik IMO nice improvements ready to go, any option?
@dimitry-ishenko amazing. even deprecated that like "pssst it was but a thing", lol NIce.

@igorpecovnik
Copy link
Member

Sorry for tuning in late. I was busy in other segments those days ... Great work! I would remove function tools/modules/functions/check_if_installed.sh right away so we don't drag it along anymore if its not used.

igorpecovnik
igorpecovnik previously approved these changes Dec 16, 2024
Copy link
Member

@igorpecovnik igorpecovnik left a comment

Choose a reason for hiding this comment

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

I assume it was replaced everywhere - in this case, free to merge.

@igorpecovnik igorpecovnik added 02 Milestone: First quarter release Ready to merge Reviewed, tested and ready for merge labels Dec 16, 2024
@dimitry-ishenko
Copy link
Collaborator Author

@igorpecovnik I've removed both deprecated functions, but it looks like I need another review before I can merge.

@Tearran
Copy link
Member

Tearran commented Dec 18, 2024

@dimitry-ishenko would have pushed this but, Something in the Unit tests broke.

@igorpecovnik
Copy link
Member

igorpecovnik commented Dec 18, 2024

Something in the Unit tests broke.

It is fine now / usually unrelated. I am working to stabilize that it won't break for 3rd party reasons that much. You can probably re-run them too - if you select unit tests here in PR.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02 Milestone: First quarter release Ready to merge Reviewed, tested and ready for merge size/large PR with 250 lines or more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants