Update CONTRIBUTING.md #161
Conversation
consideRatio
force-pushed
the
pr/update-contributing
branch
2 times, most recently
from
8cf7a0e
to
76db608
Compare
.flake8
Outdated
| # W: style warnings | ||
| # C: complexity | ||
| # F841: local variable assigned but never used | ||
| ignore = E, C, W, F841 |
There was a problem hiding this comment.
Why are we chosing to ignore these now?
There was a problem hiding this comment.
I wanted to make the new dev-jupyterhub_python.py file not fail linting just because it included c, so that is why I added a .flake8 config at all. But, why did it now include ignore of the other things?
I opted to copy-paste a .flake8 config from another JupyterHub repo, in the organization we have often opted to have a relaxed flake8 configuration to not end ut being bothered by most non-failing things.
But, practically for this PR, I'm very happy to make it just to not bug us about c -- I'll remove the other opinionated changes in the flake8 config.
There was a problem hiding this comment.
I'm not opposed to this on principle. If it makes it more compatible / smooth within the organisation, that's a good reason to have it. I was just curious.
(I, for one, wouldn't mind a slightly longer maximum line length, I run into that a lot)
There was a problem hiding this comment.
(I, for one, wouldn't mind a slightly longer maximum line length, I run into that a lot)
Oh I just ran into that as well and was a bit frustrated ^^
I'll let it be like this then, we typically have this to catch a few issues and then black to enforce formatting matters. We can do a pass on this kind of changes later, for now I'll make this as a minimal change as possible just introducing the builtin = c thing to avoid failures about the c variable from the traitlets library that makes it available when the the config file is parsed by an application.
dev-jupyterhub_config.py
Outdated
| # c.NativeAuthenticator.recaptcha_secret = "your secret" | ||
|
|
||
| # import re | ||
| # c.NativeAuthenticator.allow_self_approval_for = re.compile('[^@]+@example\.com$') |
There was a problem hiding this comment.
I actually recently identified a problem with this option.
This should just be the string and doesn't need the import of re. Compare commit 0861461.
dev-jupyterhub_config.py
Outdated
| # ------------------------------------------------------------------------- | ||
|
|
||
| c.NativeAuthenticator.check_common_password = False | ||
| c.NativeAuthenticator.minimum_password_length = 0 |
There was a problem hiding this comment.
Why do we set these to 0 and 0 instead of something like 8 and 3?
There was a problem hiding this comment.
I made it be the default value written out explicitly, but i can absolutely make it be 8 for example.
There was a problem hiding this comment.
Do you think it might be prudent to change the defaults also? 🤔
There was a problem hiding this comment.
Hmmmm, well probably, but i'm not sure to what value to set. I know password hashes can be brute forced quite easily if its just a few characters in the password. I would be cautious about using something less than 8 if i wanted to ensure that a breached password hash list wouldn't make passwords reverse-engineered from the hashes.
But yeah I think its better to have a reasonable default, like 8, than to have it set to 0. At the same time, its a bit troubling to introduce a breaking change like this. I think I'd prefer to just leave it as it is and focus on documenting things about it for a informed decision by some admin.
|
Thanks @lambdaTotoro for your review! I'll get back to address things soon (< 24 hours I'm thinking)! |
|
Take your time! You're on a roll recently! |
consideRatio
force-pushed
the
pr/update-contributing
branch
from
76db608
to
fa44ed3
Compare
|
Thanks for the quick review @lambdaTotoro! I've force pushed updated commits, just addressing the things discussed in the review comments. |
|
The new stuff looks good. Merging now. Thank you again for your valuable work! :) |
I setup to do some local development but noted that the CONTRIBUTING.md section wasn't fully updated with the actual requirements for local development.
PR changes
cvariable.