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

Refactor IPA users and groups, make the data structure make sense #16536

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Refactor IPA users and groups, make the data structure make sense #16536

merged 2 commits into from
Nov 5, 2024

Conversation

lhellebr
Copy link
Contributor

No description provided.

@lhellebr lhellebr requested review from a team as code owners September 30, 2024 11:07
@lhellebr
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_ldap_authentication.py -k test_userlist_with_external_admin -k test_verify_group_permissions

@lhellebr lhellebr added CherryPick PR needs CherryPick to previous branches 6.15.z Introduced in or relating directly to Satellite 6.15 6.16.z Introduced in or relating directly to Satellite 6.16 labels Sep 30, 2024
@pnovotny
Copy link
Contributor

PRT failed with:

dynaconf.validator.ValidationError: ipa.users is required in env main

@lhellebr
Copy link
Contributor Author

lhellebr commented Oct 1, 2024

Yes, I'm waiting for a config PR merge

@lhellebr
Copy link
Contributor Author 

trigger: test-robottelo
pytest: tests/foreman/ui/test_ldap_authentication.py -k test_userlist_with_external_admin -k test_verify_group_permissions
env:
  ROBOTTELO_fam__server__external_usergroup_name: satadmins
  ROBOTTELO_ipa__users__user: satuser_01
  ROBOTTELO_ipa__users__admin: satadmin_01
  ROBOTTELO_ipa__groups__users: satusers
  ROBOTTELO_ipa__groups__admin: satadmins

@lhellebr
Copy link
Contributor Author 

trigger: test-robottelo
pytest: tests/foreman/ui/test_ldap_authentication.py -k test_userlist_with_external_admin -k test_verify_group_permissions
env:
  ROBOTTELO_fam__server__external_usergroup_name: satadmins
  ROBOTTELO_ipa__users__user: satuser_01
  ROBOTTELO_ipa__users__admin: satadmin_01
  ROBOTTELO_ipa__groups__users: satusers
  ROBOTTELO_ipa__groups__admins: satadmins

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9040
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_ldap_authentication.py -k test_userlist_with_external_admin -k test_verify_group_permissions --external-logging
Test Result : ========== 1 passed, 53 deselected, 62 warnings in 677.19s (0:11:17) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 21, 2024
@lhellebr lhellebr requested a review from a team October 21, 2024 10:19
pnovotny
pnovotny approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@pnovotny pnovotny left a comment

Choose a reason for hiding this comment

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

LGTM

Gauravtalreja1
Gauravtalreja1 reviewed Oct 24, 2024
View reviewed changes
tests/foreman/ui/test_ldap_authentication.py
Comment on lines -111 to -113
default_ipa_host.add_user_to_usergroup(idm_users[1], sat_users[0])
yield
default_ipa_host.remove_user_from_usergroup(idm_users[1], sat_users[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it required now to add satuser_01 in the satadmin group? If not, would you mind describing why?

Copy link
Contributor Author

@lhellebr lhellebr Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

User satadmin is in satusers group, that is stable and doesn't need to be done everytime. I don't see why satuser should be in satadmins.
Although you made me realise that one of the users in tests should actually be satadmin for the test to cover what it should cover, so thanks for this question!
Also, I will rerun PRT.

@lhellebr
Copy link
Contributor Author 

trigger: test-robottelo
pytest: tests/foreman/ui/test_ldap_authentication.py -k test_userlist_with_external_admin -k test_verify_group_permissions
env:
  ROBOTTELO_fam__server__external_usergroup_name: satadmins
  ROBOTTELO_ipa__users__user: satuser_01
  ROBOTTELO_ipa__users__admin: satadmin_01
  ROBOTTELO_ipa__groups__users: satusers
  ROBOTTELO_ipa__groups__admins: satadmins

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 30, 2024
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 9188
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_ldap_authentication.py -k test_userlist_with_external_admin -k test_verify_group_permissions --external-logging
Test Result : ========== 1 passed, 53 deselected, 62 warnings in 625.42s (0:10:25) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 30, 2024
@lhellebr lhellebr requested review from Gauravtalreja1 and a team October 30, 2024 12:42
@lhellebr
Copy link
Contributor Author

lhellebr commented Nov 5, 2024

This has been open since september, can someone please click the green button?

@pondrejk pondrejk added the AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing label Nov 5, 2024
@pondrejk pondrejk merged commit ac82700 into SatelliteQE:master Nov 5, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 5, 2024
@lhellebr @web-flow
Refactor IPA users and groups, make the data structure make sense (#1…
978f7fb 
…6536)

* Refactor IPA users and groups, make the data structure make sense

* Use admin for the test, admin is in both groups

(cherry picked from commit ac82700)
@Satellite-QE Satellite-QE mentioned this pull request Nov 5, 2024
Merged
github-actions bot pushed a commit that referenced this pull request Nov 5, 2024
@lhellebr @web-flow
Refactor IPA users and groups, make the data structure make sense (#1…
64d3b14 
…6536)

* Refactor IPA users and groups, make the data structure make sense

* Use admin for the test, admin is in both groups

(cherry picked from commit ac82700)
@Satellite-QE Satellite-QE mentioned this pull request Nov 5, 2024
Merged
github-actions bot pushed a commit that referenced this pull request Nov 5, 2024
@lhellebr @web-flow
Refactor IPA users and groups, make the data structure make sense (#1…
0141cb2 
…6536)

* Refactor IPA users and groups, make the data structure make sense

* Use admin for the test, admin is in both groups

(cherry picked from commit ac82700)
@Satellite-QE Satellite-QE mentioned this pull request Nov 5, 2024
Merged
github-actions bot pushed a commit that referenced this pull request Nov 5, 2024
@lhellebr @web-flow
Refactor IPA users and groups, make the data structure make sense (#1…
325f2c0 
…6536)

* Refactor IPA users and groups, make the data structure make sense

* Use admin for the test, admin is in both groups

(cherry picked from commit ac82700)
@Satellite-QE Satellite-QE mentioned this pull request Nov 5, 2024
Merged
@vsedmik vsedmik mentioned this pull request Nov 5, 2024
Merged
@damoore044
Copy link
Contributor

@lhellebr With these newly merged changes, locally (6.16.0) I am now seeing at start of test session
dynaconf.validator.ValidationError: ipa.users is required in env main
Full Stacktrace , dropping the commit fixed locally for me. Any ideas?

@lhellebr
Copy link
Contributor Author

lhellebr commented Nov 6, 2024

@damoore044 do you have your config up to date? Config PR 1501.

@devendra104 devendra104 mentioned this pull request Nov 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pondrejk pondrejk pondrejk approved these changes

@pnovotny pnovotny pnovotny approved these changes

@Gauravtalreja1 Gauravtalreja1 Awaiting requested review from Gauravtalreja1

Assignees

@lhellebr lhellebr

Labels
6.15.z Introduced in or relating directly to Satellite 6.15 6.16.z Introduced in or relating directly to Satellite 6.16 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lhellebr @pnovotny @Satellite-QE @damoore044 @pondrejk @Gauravtalreja1