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 AlmaLinux support #98

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add AlmaLinux support #98

wants to merge 5 commits into from

Conversation

eabdullin1
Copy link

Hello!

These changes add support for AlmaLinux.

We have successfully tested it on AlmaLinux versions 8 and 9, as well as CentOS 8 Stream and CentOS 9 Stream, and can confirm that everything is working as expected.

Going forward, our plan is to continue to contribute to this project. Specifically, we aim to add more tests to ensure robustness and reliability.

Thank you for your continued support.

* Add AlmaLinux support

* Fix default page on AL9
@andrewlukoshko
Copy link

@carlosrodfern @AdamSaleh Could you please check this PR?

Copy link
Collaborator

@bstinsonmhk bstinsonmhk left a comment

Choose a reason for hiding this comment

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

Overall looks OK to me. I think in general, if we want t_functional to be useful in other places we may want to start factoring as much OS specific things into the initialization steps.

The Stream team may need to take a closer look to be sure this works properly in the CentOS Stream test harnesses.


file /etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS* >/dev/null 2>&1 && \
file /etc/pki/rpm-gpg/RPM-GPG-KEY-CentOS-Security* >/dev/null 2>&1
if [[ $is_almalinux == "yes" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use $os_name here instead of hard-coding the paths?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will try to use this way

@@ -5,9 +5,15 @@ t_Log "Running $0 - Verifying that grub2-efi is correctly signed with correct c

arch=$(uname -m)

signing_key='CentOS Secure Boot Signing 202'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we factor these into their own vars in 0_lib?

Copy link
Author

Choose a reason for hiding this comment

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

Do we really should put it to 0_lib?
I used it only for reusable vars, but this signing_key var is used only in this file

if [[ $is_almalinux == "yes" ]]; then
key="AlmaLinux ${id} signing key"
else
key="CentOS \(Linux \)\?${id} signing key"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here, we might want to factor the OS-related vars into their own groups in 0_lib

Copy link
Author

Choose a reason for hiding this comment

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

Same as in previous

@AdamSaleh
Copy link
Contributor

@eabdullin1 so, I have tested this on x86 vm on against stable composes of CentOS Stream 8 and 9, it looks it doesn't interfere with any of our tests, so if you'd do those few refactors Brian suggested, we should have no problems merging it.

I think, w.r.t. those vars that are specific to single files (key, singing_key, e.t.c), sometimes you can make test more generic, i.e. instead of

 lines_to_check="ALMALINUX_MANTISBT_PROJECT=\"AlmaLinux-$centos_ver\" ALMALINUX_MANTISBT_PROJECT_VERSION=\"$centos_ver.$minor_ver\""

you could use something like

lines_to_check="${os_name^^}_MANTISBT_PROJECT=\"$os_name-$centos_ver\" ${os_name^^}_MANTISBT_PROJECT_VERSION=\"$centos_ver.$minor_ver\"

For things that are harder to do like this, we would still prefer to have them in one place, so that we can see where we diverge and the rest of the tests are not populated by vendor-specific special cases.

We don't mind it is only used once. Still will prefer to see all of these somewhere top-level in the 0_lib/functions.sh file.

After that is done I would run it in more of our testing environments and after that works, I think we are good to go.

@eabdullin1
Copy link
Author

Ok, thank you for the reply, will fix it

@carlosrodfern
Copy link
Contributor

@AdamSaleh ,
The only test failing I saw was the firefox one. I opened an issue to address it in firefox itself: https://issues.redhat.com/browse/RHEL-2409. I provided MRs as well with a fix that uses the HOME_URL from /etc/os-release, so if it is accepted, that test will need to be updated. It could also remove the need of setting the website variable if Alma startup page is the same as the HOME_URL one.

@eabdullin1 eabdullin1 temporarily deployed to centosduffy September 14, 2023 13:33 — with GitHub Actions Inactive
@eabdullin1
Copy link
Author

@AdamSaleh @carlosrodfern Guys, can you please take a look?

@carlosrodfern
Copy link
Contributor

carlosrodfern commented Sep 22, 2023

@eabdullin1 ,
A suggestion, how about removing the "is_almalinux" all together and instead use vars and flags set in a case/esac statement? I think this MR is very close to achieve that.

For example, in the functions.sh file, grab the OS from the ID variable in /etc/os-release, and then use it in a case to set variables and flags, for instance, to skip the z tests.

os_id=$(source /etc/os-release; echo $ID)

case $os_id in
	almalinux)
                # almalinux variables here
		should_skip_z_tests=1
		;;
	centos)
		# variables here
		;;
	*)
		# default variables here
		;;
esac

Then,

  if [ -z "$should_skip_z_tests" ]; then
    t_Process <(/usr/bin/find ./tests/z_*/ -type f|sort -t'/' )
  fi

... or something along those lines.

@eabdullin1
Copy link
Author

eabdullin1 commented Oct 3, 2023

@eabdullin1 , A suggestion, how about removing the "is_almalinux" all together and instead use vars and flags set in a case/esac statement? I think this MR is very close to achieve that.

For example, in the functions.sh file, grab the OS from the ID variable in /etc/os-release, and then use it in a case to set variables and flags, for instance, to skip the z tests.

os_id=$(source /etc/os-release; echo $ID)

case $os_id in
	almalinux)
                # almalinux variables here
		should_skip_z_tests=1
		;;
	centos)
		# variables here
		;;
	*)
		# default variables here
		;;
esac

Then,

  if [ -z "$should_skip_z_tests" ]; then
    t_Process <(/usr/bin/find ./tests/z_*/ -type f|sort -t'/' )
  fi

... or something along those lines.

@carlosrodfern
Using this way is not a problem, but I don't know what should be used as default.
Probably it will be better to use centos as default variables.

os_id=$(source /etc/os-release; echo $ID)

vendor="centos"
os_name="CentOS"
grub_sb_token='CentOS Secure Boot Signing 202'
kernel_sb_token="CentOS Secure Boot Signing 201"
key_template="CentOS \(Linux \)\?%s signing key"
firefox_start_page="www.centos.org"

case $os_id in
    almalinux)
        # AlmaLinux variables
        vendor="almalinux"
        os_name="AlmaLinux"
        grub_sb_token='AlmaLinux OS Foundation'
        kernel_sb_token=$grub_sb_token
        key_template="AlmaLinux %s signing key"
        firefox_start_page="www.almalinux.org"
        ;;
    *)
        # Keep default(CentOS) variables
        ;;
esac

@carlosrodfern
Copy link
Contributor

@eabdullin1 ,
Yeah, that's fine. You could also have an entry for centos in the case, and if centos nor almalinux is found, simply exit 1 with "unsupported os" error. However, I defer to @AdamSaleh on that one since he is a collaborator.

More than the case/esac, the most important point is the removal of distro specific variable naming outside of functions.sh, to make the test suite as agnostic of EL distro as possible. I think it would be cleaner to remove the is_almalinux var and use instead flags that enable and disable things, e.g., should_skip_z_tests, in this specific case (which is the only one left):

  # For now we skipping these tests on AlmaLinux
  if [[ $is_almalinux == "no" ]]; then
    t_Process <(/usr/bin/find ./tests/z_*/ -type f|sort -t'/' )
  fi

@eabdullin1
Copy link
Author

@carlosrodfern Sorry for the huge delay, I updated functions.sh, please review it)

@carlosrodfern
Copy link
Contributor

@eabdullin1,
I just want to let you know that this conversation is also happening in parallel: https://discussion.fedoraproject.org/t/contributing-compose-tests-to-centos-stream

It will increase the ability of AlmaLinux to reuse tests and contribute if AlmaLinux migrates to tmt as well.

yuravk pushed a commit to AlmaLinux/compose-tests that referenced this pull request May 30, 2024
yuravk pushed a commit to yuravk/compose-tests that referenced this pull request Sep 24, 2024
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.

5 participants