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

Update pkg_manager.erb to include support/detection for AmazonLinux 2 and AmazonLinux 2022 #9469

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

imphocused
Copy link
Contributor

@imphocused imphocused commented Oct 18, 2022

This provides detection for AmazonLinux 2022 (dnf) and fallback 'yum' support for Amazon Linux 2, as well as other RHEL'ish clones which use /etc/system-release.

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Could you also update the comment that starts on line 16?

@@ -31,6 +31,10 @@ elif [ -f /etc/redhat-release ] ; then
else
PKG_MANAGER='yum'
fi
elif [ -f /etc/amazon-linux-release ]; then
PKG_MANAGER='dnf'
elif [ -f /etc/system-release ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Out of interest: which other systems use /etc/system-release? And we already source /etc/os-release if it's present. Can't we rely on those variables?

Copy link
Contributor Author

@imphocused imphocused Oct 20, 2022

Choose a reason for hiding this comment

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

/etc/os-release is available, but the snippet doesn't recognize yum in Amazon Linux 2.

The current RHEL clones support /etc/system-release (alma,rocky,amzn). Fedora and RHEL obviously but those already have explicit support/detection.

I found the reference from this post on Redhat, Proper way to extract Product, Release, Update, etc which discusses OS detection

3) System Release, vendor string: /etc/system-release (also via symlink, or legacy, /etc/fedora-release, /etc/redhat-release)

The response links to a sysconfcollect module that identifies some 50+ distros near the bottom of the page. I didnt check any of them because I don't use them, though I don't expect support for them in Foreman/Katello, as I don't imagine them seeing much use (if any) in Foreman installations.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we detect /etc/system-release can we use dnf instead of the yum?
Tested on Rocky & Alma and they both have dnf.

Copy link
Member

Choose a reason for hiding this comment

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

Amazon Linux 2 still has yum since it's EL7 based. Amazon Linux 2022 is based on Fedora so it has dnf.

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

Can you update the commit message? Update pkg_manager.erb is just too generic and doesn't say much about the actual changes.

@@ -31,6 +31,10 @@ elif [ -f /etc/redhat-release ] ; then
else
PKG_MANAGER='yum'
fi
elif [ -f /etc/amazon-linux-release ]; then
PKG_MANAGER='dnf'
elif [ -f /etc/system-release ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

If we detect /etc/system-release can we use dnf instead of the yum?
Tested on Rocky & Alma and they both have dnf.

@imphocused imphocused changed the title Update pkg_manager.erb Update pkg_manager.erb to include support/detection for AmazonLinux 2 and AmazonLinux 2022 Oct 20, 2022
@stejskalleos
Copy link
Contributor

@ekohl Changes LGTM, do you have any other comments?

@ekohl
Copy link
Member

ekohl commented Oct 25, 2022

It still needs a Redmine issue and the commit should refer to it. Otherwise we can't merge it (our changelog generation relies on it). Other than that I'd be happy to accept it.

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

@imphocused I see that you changed the title in github but the commit message is still same: Update pkg_manager.erb

Commit message should be in format Fixes #35670 - Description of changes, where the number is id of the issue in our tracker system.

I created the #35670 issue for you, so please just update the message, for example like this: Fixes #35670 - pkg_manager.erb - support AmazonLinux 2 & 2022

This provides detection for AmazonLinux 2022 (dnf) and fallback support for Amazon Linux 2 (yum), as well as other RHEL'ish clones
@imphocused
Copy link
Contributor Author

I've updated and pushed the change, sorry for the delay.

@stejskalleos
Copy link
Contributor

LGTM from my side, @ekohl any other comments?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I really hate it when comments are out of sync with the real code so I'd still appreciate it if the comment was updated (or removed):

# Get OS package manager
# ---
# apt-get Debian
# Ubuntu
# dnf Fedora
# RHEL family version > 7
# yum RHEL family version < 8
# pacman Arch
# zypper openSUSE Tumbleweed

@stejskalleos
Copy link
Contributor

@imphocused any updates? Updating comment is last thing before we can merge

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

🍏 LGTM

@stejskalleos
Copy link
Contributor

@ekohl any other comments? There are two commits now but I'll squash them before the merge

@ekohl ekohl merged commit 8e193d1 into theforeman:develop Dec 1, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants