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
Add coverage for UnreservedUsernameValidator
#25590
Add coverage for UnreservedUsernameValidator
#25590
Conversation
b73182a
to
c2f373a
Compare
I don't see that CI failure locally and can't replicate with a few runs and modifications of things ... will keep attempting. |
Only guess is that it's related to the environment variables set here mastodon/.github/workflows/test-ruby.yml Lines 106 to 110 in 9caa047
Those came from CircleCI, and I never got a separate PAM-only job working when I tried before |
Ah, thanks! -- it is indeed something in that block. When I set those all locally I'm able to replicate the failure. I'll sort through that and see what's happening. |
Can you use |
c2f373a
to
d206aaf
Compare
Some of them are related to what gems are loaded in the first place or what happens at initialization time of the whole app, and can't (or at least not as straightforwardly) be handled that way ... others, yes maybe. |
Just pushed a change which I think may resolve things. In my initial PR I had (erroneously!) changed the original code away from what it was doing before, which was that even if/when PAM was enabled, to still always check the reserved usernames in the settings. My initial PR changed this to either check the PAM settings or the site settings, but not both, which would have been a regression and I'm glad that CI run caught it. I pushed something which restores the original behavior which is to let either pam OR the site settings flag a reserved name. Will watch CI to confirm that was the issue. |
The code path in the PAM/Rpam2 case was previously not exercised in the suite. While in there, I did some lightweight method-rename refactor on the validator itself. As far as I can tell the behavior is the same, and coverage is now 100%.