Improve contributing docs aka hacking #57645
Conversation
dwoz
added
Needs-Testcase
dwoz
removed
the
Needs-Testcase
sagetherage
changed the title
waynew
force-pushed
the
improve-contributing-docs
branch
from
2e122df
to
81dc988
Compare
There was a problem hiding this comment.
EDIT: Conversation with @sagetherage shows that my review comments here warrant a follow-up PR because of needed discussion with others down the pipeline around release generation where we need to ensure filename changes and deletion of files doesn't impact the build/release pipeline. I'm going to create a follow-up issue.
I'm wondering whether we can do the following:
- Delete https://github.com/saltstack/salt/blob/master/.github/CONTRIBUTING.md (what GitHub is defaulting to as the contributing guide of the repo, in places like https://github.com/saltstack/salt/contribute and https://github.com/saltstack/salt/community)
- Rename
HACKING.mdtoCONTRIBUTING.md, so that GitHub automatically picks it up as the default contributing guide for people new to the repo, and so that people looking at the repo can automatically see that they should be looking at that (HACKING.mdis unclear, especially since there co-exists aCONTRIBUTING.rstthat this would also be replacing, I imagine) - Delete
rstduplicate ofHACKING.rst - Delete
CONTRIBUTING.rst - Replace references of
HACKING.rstandCONTRIBUTING.rst, where they appear in the repo, withHACKING.md(HACKING.mdis not referenced by any other files in the repo):
# HACKING.rst -> CONTRIBUTING.md
pkg/suse/salt.spec:782:%doc LICENSE AUTHORS README.rst HACKING.rst README.SUSE
MANIFEST.in:4:include HACKING.rst
# CONTRIBUTING.rst -> CONTRIBUTING.md
MANIFEST.in:3:include CONTRIBUTING.rst
That, or we go with
CONTRIBUTING.rst, and have the file get pulled in as the new contents of https://docs.saltstack.com/en/latest/topics/development/hacking.html (and rst include call), where we then will have the contributing guide in both the GitHub repo root (for easy access and reference via GitHub or source code), and in the Sphinx-generated documentation, without having duplicate files.
This would do some nice, significant cleanup and reduce a lot of confusion. Either it should be done within this PR, or in a PR directly following the merge of this PR.
HACKING.md
Outdated
| brew update | ||
| brew install pyenv | ||
| brew install pyenv-virtualenv | ||
|
|
There was a problem hiding this comment.
Chore: We might want to ask @twangboy to write the section about how to set up a Windows environment.
There was a problem hiding this comment.
Maybe open a separate issue for this chore and you can resolve this comment.
HACKING.md
Outdated
|
|
||
| So you want to contribute to the Salt project? Excellent! You can help in a | ||
| number of ways: | ||
|
|
There was a problem hiding this comment.
NOTE: I use the conventional comments method of PR reviewing.
Chore: Let's define what contributing means and what kinds of contributions we're looking for. I'm a big fan of the All Contributors Project statement that advocates for "recognizing contributors to an open source project in a way that rewards each and every contribution, not just code."
I don't think this is actually on you to do this in this PR, which is why I marked it as a follow-up chore. We need to maybe need to add this to the Open agenda and discuss it and then document the decision here. We could even go so far as considering adding the All Contributor's bot to our project, but that would require an SEP for sure. Could you help me open a ticket it or add it to the appropriate agenda and make sure I'm invited wherever that discussion occurs?
|
Let's try this one 👍 |
| If the value is less than 2047, you should increase it with: | ||
|
|
||
| SaltStack uses several labeling schemes to help facilitate code contributions | ||
| and bug resolution. See the :ref:`Labels and Milestones | ||
| <labels-and-milestones>` documentation for more information. | ||
| :: | ||
|
|
||
| Mentionbot | ||
| ---------- | ||
| ulimit -n 2047 | ||
| # For c-shell: | ||
| limit descriptors 2047 |
There was a problem hiding this comment.
I did an audit of other open PRs, just in case there may need to be additional edits here, and I noticed this in #56391:
The ulimit value is being adjusted to 3072. I'm not sure whether this is required or not, as I haven't gone this deep into the development where I was needing to adjust my ulimit -n value away from my dev system default of 1024 to 2047 or greater.
Whatever we decide on that (to update the value or not), we could actually close out #56391 because the rest of that PR is specific to Python 2, which is no longer supported by current versions of Salt. I've closed out #58128 as this PR will also supersede the need for it.
| `Git <https://git-scm.com/>`__, a common version control tool used | ||
| across many open source projects, and is necessary for contributing to | ||
| salt. For an introduction to Git, watch `Salt Docs Clinic - Git For the | ||
| True |
There was a problem hiding this comment.
Nitpick (blocking): I recommend always capitalizing Salt.
| Sending a GitHub pull request | ||
| ----------------------------- | ||
| Sweet! Now you’re ready to clone salt so you can start hacking away! If | ||
| you get stuck at any point, check out the resources at the beginning of |
There was a problem hiding this comment.
Nitpick (blocking): I recommend always capitalizing Salt.
|
|
||
| .. code-block:: bash | ||
| This creates a shallow clone of salt, which should be fast. Most of the | ||
| time that’s all you’ll need, and you can start building out other |
There was a problem hiding this comment.
Nitpick (blocking): I recommend always capitalizing Salt.
| enables us to run multiple different test configurations, as well as | ||
| other common tasks. You can think of it as Make with superpowers. | ||
| Pre-commit does what it sounds like - it configures some Git pre-commit | ||
| hooks to run ``black`` for formatting, ``isort`` for keeping our imports |
There was a problem hiding this comment.
Minor nitpick (non-blocking): Can you change the dash to a colon to satisfy my inner grammar nerd, please? :)
"Pre-commit does what it sounds like: it configures some Git pre-commit.."
| .. code-block:: bash | ||
| .. warning:: | ||
| Currently there is an issue with the pip-tools-compile pre-commit hook on windows. | ||
| The details around this issue are included here: |
There was a problem hiding this comment.
Nitpick (blocking): Please capitalize Windows. (But for the record, I care more about it for Salt than Windows.)
| The first time this will take a while because there are a *lot* of | ||
| modules. Maybe you should go grab some dessert if you already finished | ||
| that sandwich. But once Sphinx is done building the docs, python should | ||
| launch your default browser with the URL |
There was a problem hiding this comment.
Nitpick (blocking): I recommend capitalizing Python.
There was a problem hiding this comment.
In this case I'll disagree: it's python the executable, not Python the language, that's launching the browser.
There was a problem hiding this comment.
Sounds good to me. I'll defer to you. :)
| https://help.github.com/en. | ||
| If your change is a doc-only change, you can go ahead and commit/push | ||
| your code and open a PR. You can indicate that it’s a doc-only change by | ||
| adding ``[Documentation]`` to the title of your PR. Otherwise you’ll |
There was a problem hiding this comment.
Minor nitpick (non-blocking): Can you say "docs-only" instead with the plural?
"If your change is a docs-only change, you can go ahead and commit/push
your code and open a PR. You can indicate that it’s a docs-only change by..."
| ---------------------- | ||
| If you’d rather not use ``kill``, you can have a couple of terminals | ||
| open with your salt virtualenv activated and omit the ``--daemon`` | ||
| argument. Salt will run in the foreground, so you can just use ctrl+c to |
There was a problem hiding this comment.
Nitpick (blocking): I recommend capitalizing Salt here too. "...your Salt virtualenv..."
There was a problem hiding this comment.
I'll disagree here too: I'm referring to the name of the virtualenv, salt.
There was a problem hiding this comment.
I'll defer to you. That's fine.
| And that’s all that would go into your file. When it comes to your | ||
| commit message, it’s usually a good idea to add other information - what | ||
| does a reviewer need to know about the change that you made? If someone | ||
| isn’t an expert in this area, what will they need to know? | ||
|
|
There was a problem hiding this comment.
Formatting recommendation (non-blocking): I like putting things in bullet points when I have a list of two or more things. I recommend:
And that’s all that would go into your file. When it comes to your
commit message, it’s usually a good idea to add other information, such as
- What does a reviewer need to know about the change that you made?
- If someone isn’t an expert in this area, what will they need to know?
| passing and we’re not in a code freeze, ideally your code will be merged | ||
| that day. If you haven’t heard from your assigned reviewer, ping them on | ||
| GitHub, `irc <https://webchat.freenode.net/#salt>`__, or Community | ||
| Slack. | ||
|
|
There was a problem hiding this comment.
Question: Are you sure we can keep this commitment to review a PR in one business day? That hasn't been my experience when contributing to Salt and I just worry about making a commitment we can't keep.
There was a problem hiding this comment.
The goal is that we should at least be able to leave some sort of feedback within a business day. That's definitely not been the case typically, but that's not because it can't be done - we just haven't prioritized it.
There was a problem hiding this comment.
I like your "can-do" attitude. Feel free to leave this as-is. :)
waynew
dismissed stale reviews from krionbsd, s0undt3ch, and ScriptAutomate
via
808e18e
|
@barbaricyawps got those cleaned up! 👍 |
@barbaricyawps here's my update!
Closes #53633
See the pretty one here: https://github.com/waynew/salt/blob/improve-contributing-docs/HACKING.rst