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

Fix openstack_upgrade_available failing on unconventional source #705

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

canonical-xavd
Copy link
Contributor

@canonical-xavd canonical-xavd commented May 9, 2022

An unfortunate follow-up to #688 after digging up a relevant comment clarifying reasons being the previous PR.

Fixes openstack_upgrade_available calls unexpectedly sys.exit(1)ing, while expecting to be thrown an exception, whenever the package source specification does not contain an Openstack release codename.

Related change: https://review.opendev.org/c/openstack/charm-keystone/+/835087
Related: LP#1965966

Copy link

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

The changes look fine to me.

Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

Unfortunately, this can't be merged as it changes the behaviour of openstack_upgrade_behaviour(). Please see inline comment. Note, also that the doctring for the function also needs to be changed to indicate the new functionality available. Thanks.

Comment on lines +852 to +853
avail_vers = get_os_version_install_source(src,
raise_exception=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've spent a while staring at this, as it was going around my head, and I then realised that this is changing the nature of openstack_upgrade_availble() from the existing behaviour of erroring out (using error_out() in get_os_version_codename()) to a new behaviour of raising an exception (ValueError).

To fix this, this function (openstack_upgrade_avialble()) should also take an optional raise_exception=False parameter, and whatever calls it should pass in raise_exception=True to get the new behaviour. Otherwise, a whole load of charms may get different behaviour from what they are expecting currently.

@ajkavanagh ajkavanagh added the Incomplete For an issue, needs more information. For a PR, needs further work. label May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Incomplete For an issue, needs more information. For a PR, needs further work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants