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

[3006.x] Add support for Debian 12 #65116

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

ScriptAutomate
Copy link
Contributor

What does this PR do?

Adds Debian 12 to Salt test suite and package publishing

What issues does this PR fix or reference?

Fixes: #64223

@ScriptAutomate ScriptAutomate requested a review from a team as a code owner September 5, 2023 21:48
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Add support for Debian 12 [3006.x] Add support for Debian 12 Sep 5, 2023
@ScriptAutomate ScriptAutomate added the test:full Run the full test suite label Sep 5, 2023
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 5, 2023 22:14 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 5, 2023 22:14 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 5, 2023 22:14 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 5, 2023 22:14 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 5, 2023 22:32 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 5, 2023 22:35 — with GitHub Actions Inactive
s0undt3ch
s0undt3ch previously approved these changes Sep 6, 2023
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 08:31 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 08:31 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 08:31 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 08:31 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 08:31 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 08:31 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 11:24 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 11:24 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 11:24 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 11:24 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 11:24 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci September 6, 2023 11:24 — with GitHub Actions Inactive
@ScriptAutomate
Copy link
Contributor Author

ScriptAutomate commented Sep 8, 2023

Issues for Debian 12:

cron.service does not exist

Error running 'service.enable': Failed to enable unit: Unit file cron.service does not exist.

I'm going to troubleshoot this and see if we need to get cron installed on the AMIs, or if the test logic needs to change in regards to Debian 12.

UPDATE: Verified cron needs to be installed on the AMI. Going to regen an AMI to re-run tests against.

            =========================== short test summary info ============================
           FAILED tests/pytests/integration/ssh/test_master.py::test_service - assert 20 == 0
            +  where 20 = ProcessResult(returncode=20, stdout='{\n"localhost": {\n"stdout": "",\n"stderr": "Error running \'service.enable\': Failed to enable unit: Unit file cron.service does not exist.",\n"retcode": 1\n}\n}\n', stderr='', cmdline=['/tmp/testing/.nox/ci-test-onedir/bin/python', '/tmp/stsuite/scripts/cli_salt_ssh.py', '--ignore-host-keys', '--roster-file=/tmp/stsuite/master-0QAlf2/conf/roster', '--priv=/tmp/stsuite/sshd/client_key', '--user=root', '--config-dir=/tmp/stsuite/master-0QAlf2/conf', '--out=json', '--out-indent=0', '--log-level=critical', 'localhost', 'service.enable', 'cron'], data_key=None, data={'stdout': '', 'stderr': "Error running 'service.enable': Failed to enable unit: Unit file cron.service does not exist.", 'retcode': 1}).returncode

Failing to install nano

@s0undt3ch Can't tell if this is a problem with salt, or with the AMI.

           FAILED tests/pytests/functional/modules/test_pkg.py::test_pkg_install_port - salt.exceptions.SaltInvocationError: Path mirror+file:/etc/apt/mirrors/debian.list/pool/main/n/nano/nano_7.2-1_amd64.deb for package nano is either not absolute or an invalid protocol

@pytest.mark.destructive_test
@pytest.mark.slow_test
def test_pkg_install_port(grains, modules):
"""
test install package with a port in the url
"""
pkgs = modules.pkg.list_pkgs()
nano = pkgs.get("nano")
if nano:
modules.pkg.remove("nano")
if grains["os_family"] == "Debian":
url = modules.cmd.run("apt download --print-uris nano").split()[-4]
try:
ret = modules.pkg.install(sources=f'[{{"nano":{url}}}]')
version = re.compile(r"\d\.\d")
assert version.search(url).group(0) in ret["nano"]["new"]
finally:
modules.pkg.remove("nano")
if nano:
# If nano existed on the machine before the test ran
# re-install that version
modules.pkg.install(f"nano={nano}")

Failing to use pkgrepo for expected Debian 12 salt repo location

@s0undt3ch Am I understanding this output below correctly? Seems like a chicken/egg problem. Am I supposed to change my PR to make up for this, and to add it in sometime after a release makes these?

           FAILED tests/pytests/functional/states/pkgrepo/test_debian.py::test_adding_repo_file_signedby[With(aptsources.sourceslist)] - AssertionError: assert '' == 'deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main'
             - deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main
           FAILED tests/pytests/functional/states/pkgrepo/test_debian.py::test_adding_repo_file_keyserver_key_url[With(aptsources.sourceslist)] - AssertionError: assert '' == 'deb [arch=amd64] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main'
             - deb [arch=amd64] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main
           FAILED tests/pytests/functional/states/pkgrepo/test_debian.py::test_adding_repo_file_signedby_alt_file[With(aptsources.sourceslist)] - AssertionError: assert '' == 'deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main'
             - deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main
           FAILED tests/pytests/functional/states/pkgrepo/test_debian.py::test_adding_repo_file_signedby[Without(aptsources.sourceslist)] - AssertionError: assert '' == 'deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main'
             - deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main
           FAILED tests/pytests/functional/states/pkgrepo/test_debian.py::test_adding_repo_file_keyserver_key_url[Without(aptsources.sourceslist)] - AssertionError: assert '' == 'deb [arch=amd64] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main'
             - deb [arch=amd64] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main
           FAILED tests/pytests/functional/states/pkgrepo/test_debian.py::test_adding_repo_file_signedby_alt_file[Without(aptsources.sourceslist)] - AssertionError: assert '' == 'deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main'
             - deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] https://repo.saltproject.io/py3/debian/12/amd64/latest bookworm main

dmurphy18
dmurphy18 previously approved these changes Sep 12, 2023
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:02 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:02 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:02 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:02 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:02 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:02 — with GitHub Actions Inactive
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

So reviewed this this morning, and major additions - feel like - hit me up when it is done.
This is supposed to be adding Debian 12, so add Debian 12, and use another PR for refactoring how things are built. That way, adding a new platform is a simple C & P from the last for whoever has to add a platform later.

Lastly, these files have way too much

      nox-version: 2022.8.7
      python-version: "3.10"

use of magic numbers. When the change from to a different version for nox or python, invariably one entry will be missed. This has been bad practice for decades, and should be fixed with global, or variable at head of file. Understand inherited this, but that is a problem waiting to happen.

@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:55 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:55 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:55 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:55 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:55 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 9, 2023 20:55 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 10, 2023 00:32 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 10, 2023 00:32 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 10, 2023 00:32 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 10, 2023 00:32 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 10, 2023 00:32 — with GitHub Actions Inactive
@ScriptAutomate ScriptAutomate temporarily deployed to ci October 10, 2023 00:32 — with GitHub Actions Inactive
@s0undt3ch
Copy link
Collaborator

s0undt3ch commented Oct 10, 2023

So reviewed this this morning, and major additions - feel like - hit me up when it is done. This is supposed to be adding Debian 12, so add Debian 12, and use another PR for refactoring how things are built. That way, adding a new platform is a simple C & P from the last for whoever has to add a platform later.

So, how do you know the OS you added passes tests, or build packages, etc, if you don't do the necessary changes to run tests through it on the PR adding it?

Lastly, these files have way too much

      nox-version: 2022.8.7
      python-version: "3.10"

use of magic numbers. When the change from to a different version for nox or python, invariably one entry will be missed. This has been bad practice for decades, and should be fixed with global, or variable at head of file. Understand inherited this, but that is a problem waiting to happen.

This is not a problem as the variables you're referring to, although it doesn't look like it, is using "magic numbers", ie, the values are templatized and when we need to change them, we change them in one single place.

@garethgreenaway garethgreenaway merged commit cdd940a into saltstack:3006.x Oct 10, 2023
816 checks passed
@ScriptAutomate ScriptAutomate deleted the add-debian-12-tests branch October 10, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test:full Run the full test suite test:pkg Run the package tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants