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

Run sanity checks in HypervisorUpgradePlanner #360

Merged

Conversation

rgildein
Copy link
Contributor

@rgildein rgildein commented Apr 5, 2024

Run sanity checks for each in HypervisorUpgradePlanner. Improve unit tests for whole HypervisorUpgradePlanner.

fixes: #343

Run sanity checks for each in HypervisorUpgradePlanner. Improve unit
tests for whole HypervisorUpgradePlanner.

fixes: canonical#343
@rgildein rgildein added bug Something isn't working medium labels Apr 5, 2024
@rgildein rgildein self-assigned this Apr 5, 2024
@rgildein rgildein requested a review from a team as a code owner April 5, 2024 11:49
@rgildein rgildein requested review from Pjack, aieri, agileshaw and jneo8 April 5, 2024 11:49
cou/steps/hypervisor.py Show resolved Hide resolved
cou/steps/hypervisor.py Outdated Show resolved Hide resolved
cou/steps/hypervisor.py Show resolved Hide resolved
@rgildein rgildein requested a review from agileshaw April 5, 2024 15:14
cou/steps/hypervisor.py Outdated Show resolved Hide resolved
@rgildein rgildein force-pushed the bug/BSENG-2264/hypervisors-sanity-checks branch from 2203add to fdbfd7d Compare April 8, 2024 11:32
@rgildein rgildein requested a review from aieri April 8, 2024 11:33
Copy link
Member

@gabrielcocenza gabrielcocenza left a comment

Choose a reason for hiding this comment

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

This comment is not a blocker, but rather a personal opinion:

  • I'm not enjoying the tweaks that we are making because of _check_mismatched_versions.
    Maybe doing the sanity check by AZ and pass units that are in a certain group from a app could makes things clear.

If we decide to continue like this I won't block but it feels to me that it's becoming more complex to understand the code.

cou/steps/hypervisor.py Outdated Show resolved Hide resolved
tests/unit/steps/test_hypervisor.py Show resolved Hide resolved
cou/steps/hypervisor.py Outdated Show resolved Hide resolved
cou/steps/hypervisor.py Outdated Show resolved Hide resolved
cou/steps/hypervisor.py Outdated Show resolved Hide resolved
@rgildein
Copy link
Contributor Author

rgildein commented Apr 9, 2024

This comment is not a blocker, but rather a personal opinion:

* I'm not enjoying the tweaks that we are making because of `_check_mismatched_versions`.
  Maybe doing the sanity check by AZ and pass units that are in a certain group from a app could makes things clear.

If we decide to continue like this I won't block but it feels to me that it's becoming more complex to understand the code.

Me neither.

Copy link
Contributor

@agileshaw agileshaw left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for the back-and-forth, but I think it was a valuable discussion.

@rgildein rgildein merged commit 17b4fdb into canonical:main Apr 11, 2024
5 checks passed
@rgildein rgildein deleted the bug/BSENG-2264/hypervisors-sanity-checks branch April 11, 2024 14:33
@rgildein
Copy link
Contributor Author

LGTM. Sorry for the back-and-forth, but I think it was a valuable discussion.

No problems, I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HypervisorUpgradePlanner needs to run sanity check for each application
4 participants