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

DOC: Update documentation to reference SSH #26343

Closed
wants to merge 5 commits into from
Closed

Conversation

ogidig5
Copy link

@ogidig5 ogidig5 commented Apr 24, 2024

ENH: Update documentation to reference SSH

I've updated the documentation in numpy/doc/source/dev/index.rst.
This change reflects the current authentication methods using SSH keys.
This pull request replaces outdated references to GitHub Usernames and Passwords.
This change improves the clarity and accuracy of the documentation

I'd appreciate it if @bmwoodruff and @InessaPawson could review these changes.
See #26343

@mattip
Copy link
Member

mattip commented Apr 24, 2024

I think we should remove all the "typical" github interactions from this page and leave only the "special" things that are specific to NumPy. This will also future-proof the page for any changes in github policy.

@bmwoodruff
Copy link
Contributor

This appears to be a small part missed in the removal of too verbose Git details at the end of last year. See #25340

Would the best course of action be to just remove the bullet point entirely?

I think there is one more lingering reference located in RELEASE_WALKTHROUGH.rst. Would a simple " You will also need to authenticate with GitHub to push the documentation (search online for details)." work to replace "You will also need a GitHub personal access token (PAT) to push the documentation" and the bullet point that follows? This removes any reference to using a PAT or SSH.

@mattip
Copy link
Member

mattip commented Apr 25, 2024

I think most all of points (1), (2) and (3) can be removed, except the part where

If your commit introduces a new feature ...

There may be more that can be deleted. This should all be rephrased to flow better.

@bmwoodruff
Copy link
Contributor

As a first time contributor to an open source project. I think parts (1), (2), and (3) are great helps. Here are some thoughts.

  • The information about tags and submodules is needed in (1).
  • Keep the links to properly formatted commit messages, testing, and standards in (2).
  • In (3), entirely remove the second bullet referring to a username and password and SSH.

The above removes the GitHub items related to authentication and leaves enough for a newcomer to hopefully jump in.

The comment I made above about RELEASE_WALKTHROUGH.rst should be ignored.
The file it's located in is a historical reference on releasing.html.

@ogidig5 , those are my thoughts.

As a different PR, or in addition to this one, here is one more thing I wish I knew when I made my first contribution.

  • The third bullet of (3) could include "and include any appropriate CI commands."
    • We could add a bullet to the bottom of the CI commands for those that only want to update the docs. Use [skip azp] [skip actions] [skip cirrus]. This comment gets added often to doc PRs. When [docs only] becomes available (see CI, MAINT: Add docs-only option to CI #26316), this could be updated.

@ogidig5 ogidig5 marked this pull request as ready for review May 3, 2024 22:06
@charris
Copy link
Member

charris commented May 4, 2024

This should be done in a separate branch rather than main, something like ogidig5:update-reference. That is, assuming linux and starting in the main branch of your fork:

git checkout -b update-reference
<edit>
<commit>
git push origin HEAD

You can then make the PR from your fork on github.

@charris
Copy link
Member

charris commented May 4, 2024

You can see the relevant branches right under the title on this page.

@charris charris changed the title Update documentation to reference SSH DOC: Update documentation to reference SSH May 4, 2024
@charris
Copy link
Member

charris commented May 4, 2024

Note that I also edited the title to add the DOC: tag.

@bmwoodruff
Copy link
Contributor

I'll work with @ogidig5 on Monday clean up his fork and submit a new clean PR on a branch. Thanks for the comments all. We can close this PR.

@charris charris closed this May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants