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

Implement pam faillock test into openQA #13796

Merged
merged 1 commit into from Dec 3, 2021

Conversation

rfan1
Copy link
Contributor

@rfan1 rfan1 commented Dec 2, 2021

I didn't run VR on ppc64le, but I think it can pass the tests on it

@rfan1 rfan1 changed the title [WIP]Implement pam faillock test into openQA Implement pam faillock test into openQA Dec 2, 2021
assert_script_run("cp /etc/pam.d/common-auth /etc/pam.d/common-auth.back");
assert_script_run("cp /etc/pam.d/common-password /etc/pam.d/common-password.back");
assert_script_run("echo '$pam_config' >> /etc/pam.d/common-auth");
assert_script_run("echo '$pam_config' >> /etc/pam.d/common-password");
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend you use variables to define files, such as /etc/pam.d/common-auth, /etc/pam.d/common-password

Copy link
Contributor Author

@rfan1 rfan1 Dec 2, 2021

Choose a reason for hiding this comment

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

I have the opposite option here, we don't need to set a variable for system configuration file, we used more lines but gain less benefit, and make code not friendly to be read. :)

If it is not a must one, I will keep current logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I understood what you mean, but defining a variable is really a good way, such as if file name changed in the future you only need to revise the definition line to update. Besides if the variable's name is well named it is also readable too :).
Anyway, it is optional you can “keep current logic". :)

tests/security/pam/pam_faillock.pm Outdated Show resolved Hide resolved
@rfan1 rfan1 force-pushed the pam_faillock branch 2 times, most recently from a317498 to af9cca6 Compare December 2, 2021 09:54
Copy link
Contributor

@Amrysliu Amrysliu left a comment

Choose a reason for hiding this comment

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

lgtm

assert_script_run("cp /etc/pam.d/common-auth /etc/pam.d/common-auth.back");
assert_script_run("cp /etc/pam.d/common-password /etc/pam.d/common-password.back");
assert_script_run("echo '$pam_config' >> /etc/pam.d/common-auth");
assert_script_run("echo '$pam_config' >> /etc/pam.d/common-password");
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I understood what you mean, but defining a variable is really a good way, such as if file name changed in the future you only need to revise the definition line to update. Besides if the variable's name is well named it is also readable too :).
Anyway, it is optional you can “keep current logic". :)

tests/security/pam/pam_faillock.pm Outdated Show resolved Hide resolved
Copy link
Contributor

@lilyeyes lilyeyes left a comment

Choose a reason for hiding this comment

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

lgtm.

Copy link
Contributor

@jouyingbin jouyingbin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the explanation.

@Amrysliu Amrysliu merged commit d756638 into os-autoinst:master Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants