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

packaging: fix errors during install-requred-packages #8606

Merged
merged 2 commits into from Apr 6, 2020

Conversation

ilyam8
Copy link
Member

@ilyam8 ilyam8 commented Apr 5, 2020

Summary

My system:

[ilyam@ilyam-pc ~]$ cat /etc/os-release
NAME="Manjaro Linux"
ID=manjaro
ID_LIKE=arch
PRETTY_NAME="Manjaro Linux"
ANSI_COLOR="1;32"
HOME_URL="https://www.manjaro.org/"
SUPPORT_URL="https://www.manjaro.org/"
BUG_REPORT_URL="https://bugs.manjaro.org/"
LOGO=manjarolinux

There are two errors during execution of install-requred-packages.sh.

  • Unknown distribution ID: manjaro
  • validate_tree_arch: command not found

First one is minor and is likely expected but it makes me thingking that something is going wrong.

This PR fixed both errors.

Component Name

packaging

Test Plan

I did no tests, i think ci should be enough.

Additional Information

…ist of known OSes of get_os_release func and sort the list alphabetically
@squash-labs
Copy link

squash-labs bot commented Apr 5, 2020

Manage this branch in Squash

Test this branch here: https://ilyam8add-manjaro-install-requ-ibxwp.squash.io

@github-actions github-actions bot added the area/packaging Packaging and operating systems support label Apr 5, 2020
@ilyam8
Copy link
Member Author

ilyam8 commented Apr 5, 2020

Probably it is expected

Supported distributions (DD):
- arch (all Arch Linux derivatives)

Feel free to wontfix && close the PR @Ferroin @prologic

I did it because i saw Unknow distribution ID message.

@ilyam8 ilyam8 changed the title packaging: add manjaro to the install-requred-packages.sh get_os_release func packaging: fix errors during install-requred-packages Apr 5, 2020
@ilyam8
Copy link
Member Author

ilyam8 commented Apr 5, 2020

I see we have only validate_tree_centos

validate_tree_centos() {
local opts=

It is not clear why we have

validate_package_trees() {
validate_tree_${tree}
}

and use it

@knatsakis knatsakis removed their request for review April 5, 2020 18:46
@ilyam8 ilyam8 removed the request for review from ncmans April 5, 2020 19:12
@prologic
Copy link
Contributor

prologic commented Apr 5, 2020

I did no tests, i think ci should be enough.

Ci is only enough if you actually modify the appropriate workflow and add Manjaro to it :)

@prologic
Copy link
Contributor

prologic commented Apr 5, 2020

@ilyam8 validate_trees() is in case there are other special systems we have to cater for and do "extra" things to modify or alter their package trees. So far this is only CentOS; but "future proof" :)

@ilyam8
Copy link
Member Author

ilyam8 commented Apr 5, 2020

Ci is only enough if you actually modify the appropriate workflow and add Manjaro to it :)

I am not sure this was needed, but take a look

we set distribution in autodetect_distribution. There is no manjaro

if [ -z "${package_installer}" ] || [ -z "${tree}" ]; then
if [ -z "${distribution}" ]; then
# we dont know the distribution
autodetect_distribution || user_picks_distribution
fi

Then we execute detect_package_manager_from_distribution

# When no package installer is detected, try again from distro info if any
if [ -z "${package_installer}" ]; then
detect_package_manager_from_distribution "${distribution}"
fi

And there we expect manjaro

detect_package_manager_from_distribution() {
case "${1,,}" in
arch* | manjaro*)
package_installer="install_pacman"

@prologic
Copy link
Contributor

prologic commented Apr 5, 2020

Fine if you don't want/need it tested regularly in CI that's okay :)

I assume this is the correct upstream Docker image to use if we change our minds later?

@ilyam8
Copy link
Member Author

ilyam8 commented Apr 5, 2020

Ci is only enough if you actually modify the appropriate workflow and add Manjaro to it :)

manjaro is arch actually. Main difference is delayed updates (1-2 weeks).

I was fixing cosmetic error to be honest 😄

@ilyam8
Copy link
Member Author

ilyam8 commented Apr 5, 2020

I assume this is the correct upstream Docker image

i guess yes, that jonathonf guy is from the manjaro team

edit: Updated 9 months ago 🤔

@ilyam8
Copy link
Member Author

ilyam8 commented Apr 5, 2020

i am still not sure in e1cec5a

if you @prologic or @Ferroin think we dont need it i remove it

@prologic
Copy link
Contributor

prologic commented Apr 6, 2020

Its fine. Get another approval from @Ferroin and merge it :)

Copy link
Member

@Ferroin Ferroin left a comment

Choose a reason for hiding this comment

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

This does need to be verified. Current approach does not work, as 'manjaro' isn't a recognized distribution name to match on later for selecting the package manager.

@ilyam8
Copy link
Member Author

ilyam8 commented Apr 6, 2020

Current approach does not work, as 'manjaro' isn't a recognized distribution name to match on later for selecting the package manager.

detect_package_manager_from_distribution() {
case "${1,,}" in
arch* | manjaro*)
package_installer="install_pacman"

@Ferroin
Copy link
Member

Ferroin commented Apr 6, 2020

Current approach does not work, as 'manjaro' isn't a recognized distribution name to match on later for selecting the package manager.

detect_package_manager_from_distribution() {
case "${1,,}" in
arch* | manjaro*)
package_installer="install_pacman"

Sorry, missed that somehow.

@ilyam8
Copy link
Member Author

ilyam8 commented Apr 6, 2020

as 'manjaro' isn't a recognized distribution name to match on later for selecting the package manager.

if it was the case, ci would fail, right? i am asking because i did no tests, i rely on ci checks

@Ferroin
Copy link
Member

Ferroin commented Apr 6, 2020

as 'manjaro' isn't a recognized distribution name to match on later for selecting the package manager.

if it was the case, ci would fail, right? i am asking because i did no tests, i rely on ci checks

Manjaro isn't in our CI checks at all. There appear to be no up-to-date Docker images for it at the moment AFAICT. Long term we should probably tr and get it added, but It's mostly just old Arch with a fancy installer and some slightly different default behavior, so it's not exactly high priority.

@ilyam8
Copy link
Member Author

ilyam8 commented Apr 6, 2020

Manjaro isn't in our CI checks at all.

i mean install-required-packages job would fail if we had no manjaro here?

🙈 ok, i understood

@ilyam8 ilyam8 merged commit 582d987 into netdata:master Apr 6, 2020
@ilyam8 ilyam8 deleted the add_manjaro_install-required-packages branch April 6, 2020 12:45
Saruspete pushed a commit to Saruspete/netdata that referenced this pull request May 21, 2020
* packaging: update install-requred-packages.sh: add `manjaro` to the list of known OSes of get_os_release func and sort the list alphabetically

* packaging: fix `validate_package_trees` func exists before executing it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/packaging Packaging and operating systems support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants