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 tests for satellite-maintain update command #14883

Merged
merged 1 commit into from
May 14, 2024

Conversation

Griffin-Sullivan
Copy link
Contributor

Problem Statement

theforeman/foreman_maintain#802 introduces the update command which will replace z stream upgrades.

Solution

Update robottelo tests that do a z stream upgrade.

Related Issues

@Griffin-Sullivan Griffin-Sullivan added No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels Apr 25, 2024
@Griffin-Sullivan Griffin-Sullivan requested review from a team as code owners April 25, 2024 13:53
@Griffin-Sullivan
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/maintain/test_upgrade.py::test_negative_pre_update_tuning_profile_check tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate
theforeman:
    foreman_maintain: 802

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6692
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/maintain/test_upgrade.py::test_negative_pre_update_tuning_profile_check tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate --external-logging
Test Result : ================== 9 warnings, 4 errors in 1765.03s (0:29:25) ==================

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Apr 25, 2024
@Griffin-Sullivan
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/maintain/test_upgrade.py::test_negative_pre_update_tuning_profile_check tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate
theforeman:
    foreman_maintain: 802

1 similar comment
@Griffin-Sullivan
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/maintain/test_upgrade.py::test_negative_pre_update_tuning_profile_check tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate
theforeman:
    foreman_maintain: 802

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6694
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/maintain/test_upgrade.py::test_negative_pre_update_tuning_profile_check tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate --external-logging
Test Result : ============ 3 failed, 1 passed, 206 warnings in 5329.36s (1:28:49) ============

@Griffin-Sullivan
Copy link
Contributor Author

Failures here actually make sense.

  1. Can't test the preupgrade test because it's making a satellite on the fly within the test not through AAP, therefore it'll never install the packit build for foreman-maintian.

  2. The capsule test for repositories validate failed because we don't have a deploy-upstream-capsule yet.

This is good to merge and I will rerun with the one test that is expected to pass.

@Griffin-Sullivan
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate[satellite]
theforeman:
    foreman_maintain: 802

1 similar comment
@Griffin-Sullivan
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate[satellite]
theforeman:
    foreman_maintain: 802

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6710
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/maintain/test_upgrade.py::test_positive_repositories_validate[satellite] --external-logging
Test Result : ================= 1 passed, 13 warnings in 2900.65s (0:48:20) ==================

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels Apr 26, 2024
Copy link
Contributor

@synkd synkd left a comment

Choose a reason for hiding this comment

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

I may be missing some context here, but based on my reading of theforeman/foreman_maintain#802 and https://projects.theforeman.org/issues/37175, it doesn't look to me like this change necessitates changing any existing tests at this point. Eric's PR notes that 'This does not remove using the upgrade command to do a z-stream "upgrade,"' and the Foreman issue is for "add[ing] a command that updates Foreman within a minor release."

To me, that means that foreman-maintain update replaces the use of e.g. foreman-maintain upgrade --target-version 6.15.z when updating all packages on a Foreman/Satellite server but not incrementing the version number. Upgrading from e.g. Satellite 6.15.0 to 6.15.1 still requires running foreman-maintain upgrade --target-version 6.15.z.

So, in my view, we definitely need the CLI command that you've added here, and we may need one or more additional tests for the update functionality, but the existing upgrade tests do not need to be changed.

Could you please explain your changes to the tests in light of this? Alternatively, if I'm off base here, can you help me understand what I'm missing?

@Griffin-Sullivan
Copy link
Contributor Author

@synkd From my understanding I thought the update command was supposed to replace the upgrade --target-version x.y.z and do z stream upgrades. I understand we won't remove being able to do z-stream upgrades with upgrade right now, but I thought the update command is being brought in to eventually replace that. Let me double check with @ehelms

@ehelms
Copy link

ehelms commented Apr 29, 2024

The goal of the new update command is to replace the upgrade --target-version method for z-stream updates. I am first adding this new update command so that we can test it and then remove the z-stream through upgrade command after we feel confident in the update command.

@Griffin-Sullivan
Copy link
Contributor Author

@synkd forgot to address

So, in my view, we definitely need the CLI command that you've added here, and we may need one or more additional tests for the update functionality, but the existing upgrade tests do not need to be changed.

I'm opting to just update these tests to use update since these were using z-stream upgrades to test other features. The main purpose of these tests is not to test that the upgrade works, so I have another task to swap our upgrade testing for z-streams to use update.

@evgeni
Copy link
Member

evgeni commented May 2, 2024

I think tests/upgrades/test_satellite_maintain.py needs updating too, as it seems to be calling satellite-maintain upgrade list-versions --disable-self-upgrade and expect X.Y.z to be in the output, which won't be there anymore.

@Griffin-Sullivan
Copy link
Contributor Author

@evgeni Since the list-versions command won't be necessary for zstream upgrades anymore, we are going to remove those tests. I will do that in a separate PR. We are good to merge this one.

@Griffin-Sullivan
Copy link
Contributor Author

Began the work for removing list-versions testing here: #14970

@synkd synkd merged commit 00ce6d0 into SatelliteQE:master May 14, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-CherryPick PR doesnt need CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR Stream Introduced in or relating directly to Satellite Stream/Master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants