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

Allow pamtest.py to work for older versions #7267

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

noelpower
Copy link
Contributor

@noelpower noelpower commented Apr 12, 2019

Please see https://bugzilla.suse.com/show_bug.cgi?id=1131989#c11 for more info.

This patch:

  1. detect if PAM is available and use that, else use the
    more modern python3-python-pam
  2. if SLE15 run pamtest.py with python3 otherwise run with python2

Signed-off-by: Noel Power noel.power@suse.com

1) detect if PAM is available and use that, else use the
   more modern python3-python-pam
2) if SLE15 run pamtest.py with python3 otherwise run with python2

Signed-off-by: Noel Power <noel.power@suse.com>
@Zaoliang
Copy link
Contributor

thanks!

if [ ! -f /etc/os-release ]; then
return 1
fi
if (grep -i -q "SUSE Linux Enterprise Server 15" /etc/os-release); then
Copy link
Member

Choose a reason for hiding this comment

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

why not source and use $VERSION_ID variable instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simply ease of implementation, there are probably many different ways to detect the version, I saw elsewhere code doing this

isSles15(){
   if [ ! -f /etc/os-release ]; then
      return 1
   fi
   .  /etc/os-release

   if [[ "$VERSION_ID" == "15"* ]]; then
      return 0
   fi
   return 1
}

should work too, I actually prefer the simple grep (but I am not in anyway comfortable writing sh scripts :-)) Please let me know if you wish me to update with the change above or if the orig code is ok

Copy link
Member

@foursixnine foursixnine Apr 15, 2019

Choose a reason for hiding this comment

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

Don't worry @noelpower :), was more out of curiosity :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks alot!!

@foursixnine foursixnine merged commit 46b3f63 into os-autoinst:master Apr 15, 2019
@noelpower noelpower deleted the bnc1131989 branch April 16, 2019 07:19
@Zaoliang
Copy link
Contributor

https://openqa.suse.de/tests/2812575#step/sssd/20 shows the fix is working, but failed at
https://openqa.suse.de/tests/2812575#step/sssd/30

my @scenario_list;
push @scenario_list, 'local' if (version->parse($sssd_version) < version->parse(2.0.0)); # sssd 2.0+ removed support of 'local'
push @scenario_list, qw(
ldap
ldap-no-auth
ldap-nested-groups
krb
);
Can you submit a new PR for ldap, ldap-no-auth, ldap-nested-groups, krb as well? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants