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

Improve Simple Content Access UX (RHEL 8) #3867

Merged
merged 4 commits into from Feb 23, 2022

Conversation

M4rtinK
Copy link
Contributor

@M4rtinK M4rtinK commented Feb 15, 2022

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.

This is the RHEL 8 version of #3828.

@pep8speaks
Copy link

pep8speaks commented Feb 15, 2022

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-22 14:26:52 UTC

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Feb 15, 2022

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

@M4rtinK M4rtinK added the blocked Don't merge this pull request! label Feb 15, 2022
Add support for detecting Single Content Access mode from data returned by
the RHSM DBus API Register() and RegisterWithActivationKeys() methods.

As the parsing function is used in two places, put it into an utils
module.

Related: rhbz#1968574
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Pylint found some issues.

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Feb 17, 2022

Pylint found some issues.

Pylint issues have been fixed up. :)

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Feb 21, 2022

/kickstart-test --testtype smoke

@jstodola
Copy link
Contributor

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

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Feb 21, 2022

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

Yeah, dropping the blocked label. :)

@M4rtinK M4rtinK removed the blocked Don't merge this pull request! label Feb 21, 2022
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me except this small nitpick.

@@ -241,13 +243,14 @@ def run(self):
# We do not yet support setting organization for username & password
# registration, so organization is blank for now.
organization = ""
private_register_proxy.Register(organization,
registration_data = private_register_proxy.Register(organization,
self._username,
self._password,
{},
{},
locale)
Copy link
Member

Choose a reason for hiding this comment

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

Indentation issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  Yep, that
s all
       wrong.

Should be fixed now.

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Please, fix the issue above. Otherwise, it looks good to me.

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#1968574
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#1968574
The necessary API support for returning Simple Content Access mode
as part of registration data & working SCA+auto-attach needs at least
1.28.26.

Related: rhbz#1968574
@M4rtinK
Copy link
Contributor Author

M4rtinK commented Feb 22, 2022

Please, fix the issue above. Otherwise, it looks good to me.

Issue has been fixed.

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Feb 22, 2022

/kickstart-test --testtype smoke

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Thanks!

@M4rtinK M4rtinK added the ready to merge The PR can be merged. It should have all BZ flags required for releasing set (usually release+). label Feb 23, 2022
@poncovka poncovka merged commit f660e40 into rhinstaller:rhel-8 Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR can be merged. It should have all BZ flags required for releasing set (usually release+). rhel8-branch
5 participants