-
Notifications
You must be signed in to change notification settings - Fork 390
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
[MRG] Improve contribution document #1251
Conversation
19084ec
to
97d17b0
Compare
One thing I've noticed when running the tests is that
The last line is printed by our unit test and indicates that it got it and it will visit the repo. The second to last line is BinderHub saying "the server is ready". So the mystery for me is why so much time (maybe 10-20seconds pass? I've not timed it but it is much more than "1-2 seconds) passes between these two. |
97d17b0
to
5b08486
Compare
- `auth`: Tests related to BinderHub's usage of JupyterHub as an OAuth2 Identity | ||
Provider (IdP) for non public access. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overview this section contain took quite some time for me to figure out by code inspection and has been important to me to comprehend - can we retain it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about having a dedicated section in the docs that contains all the nitty gritty details, how to run all the tests, other pitfalls, etc. With the goal of keeping CONTRIBUTING.md
aimed at newcomers or a quick reference.
Having such a place to store commands I actually use (because CONTRIBUTING.md uses "a Ubuntu system as common denominator" would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure go for it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to explicitly request a change I find important: https://github.com/jupyterhub/binderhub/pull/1251/files#r563198058
@betatim would you like to keep working on this? |
I won't have time to move the commands and start a new document for the nitty gritty. I think we can create a follow up issue to take care of that and merge this PR |
Can you be more specific with what you mean with "move the commands"? Before a merge, I'd like to ensure the deleted section about the tests is retained in our docs. I've copy pasted it below for clarity of what I refer to.
|
Moving that section to some new document is something I'd punt to a different PR/someone else as I won't have time to work on that. |
It is my understanding that you wish that section to be either moved or deleted. It is my wish that it is retained someplace - preferably in CONTRIBUTING.md where I think it fits best. To me, it was quite challenging to onboard myself to grasp that overview as I had to do it via code inspection. If you wish to have it moved or refactored somehow, that's okay with me, but to straight up delete it isn't. |
Feel free to make a new PR based on this one which reflects a compromise you like. |
@betatim are you asking me to accept this PR removing a section I wanted to retain, and that I submit a new PR adding this section back after this PR then is merged? |
@betatim when I read #1251 (comment) I got quite sad, and I still feel sad about the interaction we had in this PR. I remain emotional about this PR because I understand it as a the changes suggested in it stems from reverting changes I suggested in #1188 rather than iterating on the documentation. I'm willing to revert changes I've introduced, but I'm not willing to approve/merge this PR in order to create a new one myself adding them back. (EDIT: "adding them back" should have been "adding the section on running tests back") I'm sorry that you didn't get a chance to review changes in #1188 before they were merged. |
(At the time) I thought this was an improvement over what was in |
Thank you for following up with me @betatim! I'll for now go for a close here then. If you or someone else wants to work this, the wish I have is that the section I added about running tests is retained. I don't feel strongly about any language changes, but that information has been critical for my own development efforts in this repo. |
This adds back some sections that got lost over time and fixes typos, broken links, etc.
I followed all the individual guides to check that they work, except for the "helm chart development" one.