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

(MODULES-10892) Update name.pp #353

Merged
merged 4 commits into from
Mar 15, 2021
Merged

(MODULES-10892) Update name.pp #353

merged 4 commits into from
Mar 15, 2021

Conversation

LooOOooM
Copy link
Contributor

@LooOOooM LooOOooM commented Dec 9, 2020

bugfix: MODULES-10892
@LooOOooM LooOOooM requested a review from a team as a code owner December 9, 2020 18:11
@sheenaajay sheenaajay self-assigned this Jan 4, 2021
@sheenaajay
Copy link
Contributor

Thank you @LooOOooM for submitting the PR.
The unit test cases failing with the following changes.

Invalid user name values "1-bad-dude" is expected not to match value "1-bad-dude" (FAILED - 1) ".hidden" is expected not to match value ".hidden" (FAILED - 2)

Could you please check the following, the updated regex are not satisfying the following username rules.

  • Fully numeric usernames and usernames . or .. are also disallowed
  • It is not recommended to use usernames beginning with . character as their home directories will be hidden in
  • the ls output

@pmcmaw pmcmaw changed the title Update name.pp (MODULES-10892) Update name.pp Feb 8, 2021
@CLAassistant
Copy link

CLAassistant commented Feb 24, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Timo Bergemann
❌ LooOOooM


Timo Bergemann seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pmcmaw
Copy link
Contributor

pmcmaw commented Mar 8, 2021

Hey @LooOOooM :-)

Just wondering if you have any bandwidth to work on this so we can get it merged :-)

@LooOOooM
Copy link
Contributor Author

done

@pmcmaw
Copy link
Contributor

pmcmaw commented Mar 15, 2021

Hey @LooOOooM :-)

I want to thank you for taking the time to make this change however, 1-bad-dude would be an acceptable username therefore should remain in our tests. The change would be expected to go into the regex pattern to both accommodate for existing tests and also your own use case.

@LooOOooM
Copy link
Contributor Author

LooOOooM commented Mar 15, 2021

sry, I dont get it? is there anything left to do apart from finnaly getting this into production? As of course "1-bad-dude" is a valid username it should get removed out of the test, as the test is a negativ one. The test is failing as the regex is matching "1-bad-dude", However 1-bad-dude is not a fancy name:

[root@puppet-centos7-testvm1 ~]# useradd -m 1-bad-dude
[root@puppet-centos7-testvm1 ~]# echo $?
0
[root@puppet-centos7-testvm1 ~]# tail -1 /etc/passwd
1-bad-dude:x:1009:1010::/home/1-bad-dude:/bin/bash
[root@puppet-centos7-testvm1 ~]# su - 1-bad-dude

This might be for specific distriputions (like debian based) differnet, but for redhat based it is not.
Therefore I think the module should leave the restrictions to the distribution rather then forcing debian best rule out to other distribitions.

@pmcmaw
Copy link
Contributor

pmcmaw commented Mar 15, 2021

@LooOOooM

Im really sorry, I missed the fact it was a negative test.

Would you mind dropping the test into the valid usernames list here:

describe 'Valid user name values' do

Once done I will merge.

@LooOOooM
Copy link
Contributor Author

done

@pmcmaw pmcmaw merged commit 9ee0f2e into puppetlabs:main Mar 15, 2021
@pmcmaw
Copy link
Contributor

pmcmaw commented Mar 15, 2021

Thank you and again I apologize for the oversight on my behalf with the test update.

@pmcmaw pmcmaw added the bugfix label Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants