Improve Simple Content Access UX #3828

M4rtinK · 2022-02-04T17:59:23Z

Show a proper status message instead of a confusing (even if correct) message about no subscription being attached if install time registration is done in Simple Content Access mode.

This is achieved by parsing the data returned by the register methods of the RHSM DBus, exposing this via a DBus property of the Subscription module, which is then used by the GUI to display the appropriate string.

pep8speaks · 2022-02-04T17:59:27Z

Hello @M4rtinK! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-09 02:34:11 UTC

M4rtinK · 2022-02-04T18:01:34Z

This is how the Connect to Red Hat screen looks like with this PR applied after registration in SCA mode.

M4rtinK · 2022-02-04T18:02:46Z

We can't merge this until subscription-manager 1.29.24 has been released an is available n the compose. So setting the blocked label.

M4rtinK · 2022-02-07T11:21:40Z

/kickstart-test --testtype smoke

poncovka

Looks good to me.

poncovka · 2022-02-08T10:50:41Z

pyanaconda/modules/subscription/runtime.py

-                                                {},
-                                                {},
-                                                locale)
+                registration_data = private_register_proxy.Register(self._organization,


Please, move arguments of the function on the new line to fix the indentation.

registration_data = private_register_proxy.Register( self._organization, self._username, ... )

Indeed, that looks much better. :)

poncovka · 2022-02-08T10:52:26Z

pyanaconda/modules/subscription/runtime.py

                log.debug("subscription: registered with username and password")
+                return registration_data


The new return value is not documented.

Oh, right, thanks! Fixed. :)

poncovka · 2022-02-08T10:58:12Z

pyanaconda/modules/subscription/subscription.py

@@ -666,6 +692,9 @@ def _system_unregistered_callback(self):
        # script, or else it will be run by the target system
        # provisioning task
        self._set_satellite_provisioning_script(None)
+        # also when we are no longer registered then we are are
+        # thus no longer in Simple Content Access mode
+        self.set_simple_content_access_enabled(False)


All these data should be ideally represented by another object, that we could easily drop and replace by another one.

While I'm afraid this is as far as we can reasonably go given the scope and timeline of this fix, there is definitely some room for improvement in the future.

Ideally the SCA mode value would be part of the data describing attached subscriptions but that's handled much later in the flow and with a separate task from the registration one, so I didn't find an easy way of putting it there without possibly changing quite a bit of code.

Still, there is a a ray of hope in the form of a forthcoming RHSM API improvement that could potentially remove the need to call AutoAttach() and maybe even GetPools(). Then we could just have a single register task, that would do all whats needed and would return all the necessary data in one go. Would could then feed that data into a single DBus struct describing what happened and be done with it, potentially making the code much simpler and smaller. :)

jkonecny12

I found some nitpicks but other than that it looks good to me.

jkonecny12 · 2022-02-08T10:47:07Z

pyanaconda/modules/subscription/runtime.py

+        :param str registration_data_json: registration data in JSON format
+        :return: True if data inicates SCA enabled, False otherwise
+        """
+        # we can't try to detect SCA mode if we don't have any registration data data


Typo ... data data

jkonecny12 · 2022-02-08T10:49:25Z

pyanaconda/modules/subscription/runtime.py

+            elif content_access_mode == "entitlement":
+                # SCA explicitely not enabled
+                return False
+            else:
+                log.warning("contentAccessMode mode not set to known value:")
+                log.warning(content_access_mode)
+                # unknown mode or missing data -> not SCA
+                return False


Are we really interested in the value? Wouldn't it be sufficient to just log the value if it is not equal to "org_environment"?

Past experience with RHSM & the closely related Candlepin and Satellite APIs indicates a reasonable potential for unexpected values coming back from the API & that it's very useful to have debug data available when this happens.

You would still have the log there. It's just one for both branches which seems fine to me? If you want to still have info that entitlement is possible than comment in the code should be enough for that.

jkonecny12 · 2022-02-08T10:51:23Z

tests/unit_tests/pyanaconda_tests/modules/subscription/test_subscription_tasks.py

+        parse_method = RegisterAndSubscribeTask._detect_sca_from_registration_data
+        # the parsing method should be able to survive also getting an empty string
+        # or even None, returning False
+        assert not parse_method("")


You should test here

assert <something> is True/False

Otherwise anything is returned (except False and None) will be taken as correct output.

That doesn't sound right, though. Assert checks a bool value. The function returns a bool. That makes it correct, unless you also want to ensure the function returns only bools and nothing else?

I guess that you want to make that sure. The point is that False != None which is not that hard to have (missing return statement) and it could catch a bug this way.

jkonecny12 · 2022-02-08T10:52:54Z

pyanaconda/modules/subscription/runtime.py

    def __init__(self, rhsm_observer, subscription_request, system_purpose_data,
                 registered_callback, registered_to_satellite_callback,
-                 subscription_attached_callback, subscription_data_callback,
-                 satellite_script_callback, config_backup_callback):
+                 simple_content_access_callback, subscription_attached_callback,
+                 subscription_data_callback, satellite_script_callback,
+                 config_backup_callback):


Not for this PR but this is a bit too many parameters. It would be great to somehow cleanup this when backported to master.

Yeah, doing something about this is definitely planned for the upstream version of Satellite support. :)

VladimirSlavik

Mostly looks good to me, +1 to the other reviews.

I am slightly confused about how "simple content access" and "SCA" are mixed pretty freely, I think you coudl have saved a lot of typing by always using SCA, but never mind :)

Add support for detecting Single Content Access mode from data returned by the RHSM DBus API Register() and RegisterWithActivationKeys() methods. Related: rhbz#2048449

Add the IsSimpleContentAccessEnabled property to the Subscription module DBus interface. This property is set to True if SCA mode is detected after a successfully registration attempt. The property is reset back to the initial state (False) on unregistration. Related: rhbz#2048449

Instead of showing "No subscriptions attached" on the Connect to Red Hat screen in SCA mode show "Subscribed in Simple Content Access mode." instead. Resolves: rhbz#2048449

The necessary API support for returning Simple Content Access mode as part of registration data was only added in subscription-manager 1.29.23. Unfortunately this version also contains the auto-attach regression (https://bugzilla.redhat.com/show_bug.cgi?id=2049139), so we need at least 1.29.24, which should have both the necessary API support as well as the regression fixed. Related: rhbz#2048449

M4rtinK · 2022-02-21T03:28:01Z

/kickstart-test --testtype smoke

jstodola · 2022-02-21T09:33:36Z

https://bugzilla.redhat.com/show_bug.cgi?id=2049139 with subscription-manager-1.29.25-1.el9 in ON_QA, so this PR is not blocked by subscription-manager any more.

M4rtinK · 2022-02-21T11:37:31Z

/kickstart-test --testtype smoke

M4rtinK · 2022-02-21T11:39:30Z

https://bugzilla.redhat.com/show_bug.cgi?id=2049139 with subscription-manager-1.29.25-1.el9 in ON_QA, so this PR is not blocked by subscription-manager any more.

Indeed, dropping the blocked label.

M4rtinK · 2022-02-21T19:26:11Z

/kickstart-test --testtype smoke

M4rtinK · 2022-02-22T03:39:32Z

/kickstart-test --testtype smoke

M4rtinK · 2022-02-22T12:22:04Z

/kickstart-test --testtype smoke

M4rtinK · 2022-09-02T14:08:15Z

This can be ported to Fedora only once Satellite support has been ported.

M4rtinK added port to Fedora rhel-9 labels Feb 4, 2022

M4rtinK added the blocked Don't merge this pull request! label Feb 4, 2022

M4rtinK force-pushed the rhel-9-improve_SCA_UX branch 2 times, most recently from 25b2a4d to c83c904 Compare February 4, 2022 18:11

poncovka approved these changes Feb 8, 2022

View reviewed changes

jkonecny12 approved these changes Feb 8, 2022

View reviewed changes

VladimirSlavik approved these changes Feb 8, 2022

View reviewed changes

Add support for SCA mode detection from registration data

391459b

Add support for detecting Single Content Access mode from data returned by the RHSM DBus API Register() and RegisterWithActivationKeys() methods. Related: rhbz#2048449

M4rtinK force-pushed the rhel-9-improve_SCA_UX branch from c83c904 to 75411b4 Compare February 9, 2022 02:13

M4rtinK added 3 commits February 9, 2022 03:33

Improve UX in Simple Content Access mode

f173720

Instead of showing "No subscriptions attached" on the Connect to Red Hat screen in SCA mode show "Subscribed in Simple Content Access mode." instead. Resolves: rhbz#2048449

M4rtinK force-pushed the rhel-9-improve_SCA_UX branch from 75411b4 to c042039 Compare February 9, 2022 02:34

M4rtinK mentioned this pull request Feb 15, 2022

Improve Simple Content Access UX (RHEL 8) #3867

Merged

M4rtinK added ready to merge The PR can be merged. It should have all BZ flags required for releasing set (usually release+). and removed blocked Don't merge this pull request! labels Feb 21, 2022

poncovka merged commit 927dba0 into rhinstaller:rhel-9 Feb 24, 2022

M4rtinK added the blocked Don't merge this pull request! label Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Simple Content Access UX #3828

Improve Simple Content Access UX #3828

M4rtinK commented Feb 4, 2022

pep8speaks commented Feb 4, 2022 •

edited

M4rtinK commented Feb 4, 2022

M4rtinK commented Feb 4, 2022

M4rtinK commented Feb 7, 2022

poncovka left a comment

poncovka Feb 8, 2022

M4rtinK Feb 9, 2022

poncovka Feb 8, 2022

M4rtinK Feb 9, 2022

poncovka Feb 8, 2022

M4rtinK Feb 9, 2022

jkonecny12 left a comment

jkonecny12 Feb 8, 2022

M4rtinK Feb 9, 2022

jkonecny12 Feb 8, 2022

M4rtinK Feb 9, 2022

jkonecny12 Feb 10, 2022

jkonecny12 Feb 8, 2022

VladimirSlavik Feb 8, 2022 •

edited

jkonecny12 Feb 10, 2022

jkonecny12 Feb 8, 2022

M4rtinK Feb 9, 2022

VladimirSlavik left a comment

M4rtinK commented Feb 21, 2022

jstodola commented Feb 21, 2022

M4rtinK commented Feb 21, 2022

M4rtinK commented Feb 21, 2022

M4rtinK commented Feb 21, 2022

M4rtinK commented Feb 22, 2022

M4rtinK commented Feb 22, 2022

M4rtinK commented Sep 2, 2022

		log.debug("subscription: registered with username and password")
		return registration_data

Improve Simple Content Access UX #3828

Improve Simple Content Access UX #3828

Conversation

M4rtinK commented Feb 4, 2022

pep8speaks commented Feb 4, 2022 • edited

Comment last updated at 2022-02-09 02:34:11 UTC

M4rtinK commented Feb 4, 2022

M4rtinK commented Feb 4, 2022

M4rtinK commented Feb 7, 2022

poncovka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladimirSlavik Feb 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladimirSlavik left a comment

Choose a reason for hiding this comment

M4rtinK commented Feb 21, 2022

jstodola commented Feb 21, 2022

M4rtinK commented Feb 21, 2022

M4rtinK commented Feb 21, 2022

M4rtinK commented Feb 21, 2022

M4rtinK commented Feb 22, 2022

M4rtinK commented Feb 22, 2022

M4rtinK commented Sep 2, 2022

pep8speaks commented Feb 4, 2022 •

edited

VladimirSlavik Feb 8, 2022 •

edited