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.

Sign up for GitHub

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

Jump to bottom

Fix the condition for completing the root configuration (#1981807) #3701

Merged
merged 3 commits into from Jan 4, 2022
Merged

Fix the condition for completing the root configuration (#1981807) #3701

merged 3 commits into from Jan 4, 2022

Conversation

poncovka
Copy link
Contributor

Revert changes in the completed property of the root configuration spoke
that were done in the commits e1dd6aa and 862eee2. They were never necessary.

Resolves: rhbz#1981807

@poncovka
Fix the condition for completing the root configuration (#1981807)
2aa2b34 
Revert changes in the `completed` property of the root configuration spoke
that were done in the commits e1dd6aa and 862eee2. They were never necessary.

Resolves: rhbz#1981807
@poncovka
Copy link
Contributor Author

/kickstart-test --testtype smoke

@poncovka
Copy link
Contributor Author

/kickstart-test --testtype users

@poncovka poncovka added the manual testing required This issue can't be merged without manual testing label Nov 11, 2021
@VladimirSlavik
Copy link
Contributor

Looks good at a glance. However, make sure to check behavior on repeated changes from ui. The "caching" of values in the spokes and module sometimes interact weirdly.

@jkonecny12
Copy link
Member

It looks simple, elegant and correct to me. However, we really need to check if we won't break something else. Could you please run appropriate KS tests on this (maybe all of them)?

Probably a dumb question but did you tested the issue what RHEV is facing? If this will stop the installation in case the root password is not set?

@rvykydal rvykydal mentioned this pull request Nov 25, 2021
Closed
@poncovka poncovka removed the manual testing required This issue can't be merged without manual testing label Nov 25, 2021
@poncovka poncovka marked this pull request as ready for review November 25, 2021 16:30
@poncovka
Copy link
Contributor Author

I have tested the changes.

The good stuff that will be fixed with this pull request:

  • The root spokes blocks the installation on RHVH with the default kickstart file.
  • The root spoke is visible and blocks the installation for an empty kickstart file in TUI and GUI.
  • It worked this way before the rebase and it was broken since then in RHEL 8.

The questionable stuff that will change with this pull request:

  • The root spoke can be entered with rootpw --lock in a kickstart file.
  • It wasn't possible to enter the spoke in RHEL 8.
  • It is possible to enter the GUI spoke in upstream and RHEL 9.
  • It is possible to enter the TUI spoke in upstream, but not in RHEL 9 - this is a completely different bug.

@jkonecny12, @jstodola, are the changes acceptable?

@poncovka
Copy link
Contributor Author

RHVH without the fix:
RHVH_before

RHVH with the fix:
RHVH

@jstodola
Copy link
Contributor

The questionable stuff that will change with this pull request:

* The root spoke can be **entered** with `rootpw --lock` in a kickstart file.

The installation guide answers the question:

--lock - If this option is present, the root account is locked by default. This means that the root user will not be able to log in from the console. This option will also disable the Root Password screens in both the graphical and text-based manual installation.

We should not change this documented behavior.

@jstodola
Copy link
Contributor 

* It is possible to enter the GUI spoke in upstream and RHEL 9.

https://bugzilla.redhat.com/show_bug.cgi?id=2026916

@jkonecny12
Copy link
Member

Are we somehow able to change #3701 (comment) without changing other behavior? Sounds to me that it should be possible to just detect that rootpw --locked was used?

@poncovka
Copy link
Contributor Author

Are we somehow able to change #3701 (comment) without changing other behavior? Sounds to me that it should be possible to just detect that rootpw --locked was used?

It is doable. I have a suspicion that the initial setup allows to enable a locked root account, so we might need a workaround for that. There is a new commit with suggested changes, but it needs to be tested.

@poncovka poncovka added the blocked Don't merge this pull request! label Nov 29, 2021
VladimirSlavik
VladimirSlavik reviewed Nov 29, 2021
View reviewed changes
Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

LGTM, as far as I can tell. Untangling sensitive from completed is a huge win.

@poncovka
Fix the condition for entering the root configuration
6b5e8df 
Don't allow to enter the spoke if the root account is already configured
in the kickstart file in Anaconda. Just a note, the Initial Setup in the
reconfiguration mode always allows to enter this spoke.

Related: rhbz#1981807
@poncovka
Copy link
Contributor Author

poncovka commented Nov 30, 2021 •
edited

Retested with Anaconda and Initial Setup. The questionable stuff from #3701 (comment) are fixed. Initial Setup is not affected by these changes.

@poncovka
Copy link
Contributor Author

/kickstart-test --testtype smoke

@poncovka
Copy link
Contributor Author

/kickstart-test --testtype users

@poncovka poncovka added port to Fedora port to RHEL9 The pull request needs to be ported to RHEL 9. and removed blocked Don't merge this pull request! labels Nov 30, 2021
VladimirSlavik
VladimirSlavik approved these changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@VladimirSlavik VladimirSlavik 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. Thank you!

jkonecny12
jkonecny12 approved these changes Dec 2, 2021
View reviewed changes
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. Thanks!

@poncovka poncovka mentioned this pull request Dec 7, 2021
Merged
@poncovka poncovka removed the port to RHEL9 The pull request needs to be ported to RHEL 9. label Dec 7, 2021
@poncovka
Fix the status of the root configuration screen
58123ef 
If the root account is locked, show the "Root account is disabled" status.
Without the fix, Anaconda shows "Disabled, set password to enable" if there
is `firstboot --reconfig` in a kickstart file.

The workaround for the Initial Setup can be removed. It was implemented in
the commit 98d7b29 for the bug 1507940, but the bug didn't require to add
a special status.

Related: rhbz#1981807
@poncovka
Copy link
Contributor Author

poncovka commented Dec 8, 2021

/kickstart-test --testtype smoke

@poncovka
Copy link
Contributor Author

poncovka commented Dec 8, 2021

/kickstart-test --testtype users

@poncovka
Copy link
Contributor Author

poncovka commented Dec 8, 2021

I have found another bug. See the last commit, please. I planned to remove this code anyway, so let's do it now since it doesn't work properly. @jstodola, @jkonecny12, @M4rtinK

@jstodola
Copy link
Contributor

jstodola commented Dec 8, 2021

Ack.

rvykydal
rvykydal approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@rvykydal rvykydal 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.

@poncovka poncovka added the ready to merge The PR can be merged. It should have all BZ flags required for releasing set (usually release+). label Dec 13, 2021
jkonecny12
jkonecny12 approved these changes Dec 15, 2021
View reviewed changes
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.

Thanks for this fix. Looks good to me.

@M4rtinK M4rtinK merged commit 0f866ec into rhinstaller:rhel-8 Jan 4, 2022
@poncovka
Copy link
Contributor Author

poncovka commented Jan 7, 2022

Ported to upstream at #3775.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvykydal rvykydal rvykydal approved these changes

@jkonecny12 jkonecny12 jkonecny12 approved these changes

@VladimirSlavik VladimirSlavik VladimirSlavik approved these changes

Assignees
No one assigned
Labels
ready to merge The PR can be merged. It should have all BZ flags required for releasing set (usually release+). rhel8-branch
Milestone
No milestone
6 participants
@poncovka @VladimirSlavik @jkonecny12 @jstodola @rvykydal @M4rtinK