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

Improve contributing docs aka hacking #57645

Merged
merged 15 commits into from Jan 11, 2021
Merged
388 changes: 388 additions & 0 deletions HACKING.md
@@ -0,0 +1,388 @@
# Developing Salt

waynew marked this conversation as resolved.
Show resolved Hide resolved

So you want to contribute to the Salt project? Excellent! You can help in a
number of ways:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

- Using Salt and opening bug reports.
waynew marked this conversation as resolved.
Show resolved Hide resolved
- Joining a [working group](https://github.com/saltstack/community).
- Answering questions on [irc][salt on freenode], the
[community Slack](https://saltstackcommunity.herokuapp.com/), or the
[salt-users mailing list](https://groups.google.com/forum/#!forum/salt-users).
- Fixing issues.

If you'd like to update docs or fix an issue, you're going to need the Salt
repo. The best way to contribute is using [Git](https://git-scm.com/).

## Environment Setup

To hack on Salt or the docs you're going to need to setup your development
environment. If you already have a workflow that you're comfortable with, you
waynew marked this conversation as resolved.
Show resolved Hide resolved
can use that, but otherwise this is an opinionated guide for setting up your
dev environment. Follow these steps and you'll end out with a functioning dev
environment and be able to submit your first PR.

This guide assumes at least a passing familiarity with
[Git](https://git-scm.com/), and that you already have it installed and
configured.

waynew marked this conversation as resolved.
Show resolved Hide resolved
## `pyenv`, Virtual Environments, and You

We recommend [pyenv](https://github.com/pyenv/pyenv), since it allows
installing multiple different Python versions, which is important for testing
Salt across all the versions of Python that we support.

### On Linux

Install pyenv:

git clone https://github.com/pyenv/pyenv.git ~/.pyenv
git clone https://github.com/pyenv/pyenv-virtualenv.git $(pyenv root)/plugins/pyenv-virtualenv

### On Mac

Install pyenv using brew:

brew update
brew install pyenv
brew install pyenv-virtualenv

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chore: We might want to ask @twangboy to write the section about how to set up a Windows environment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe open a separate issue for this chore and you can resolve this comment.

---

Now add pyenv to your `.bashrc`:

export PATH="/root/.pyenv/bin:$PATH" >> ~/.bashrc
eval "$(pyenv init -)" >> ~/.bashrc
eval "$(pyenv virtualenv-init -)" >> ~/.bashrc

For other shells, see [the pyenv instructions](https://github.com/pyenv/pyenv#basic-github-checkout).

Go ahead and restart your shell. Now you should be able to install a new
version of Python:

pyenv install 3.7.0

If that fails, don't panic! You're probably just missing some build
dependencies. Check out [pyenv common build
problems](https://github.com/pyenv/pyenv/wiki/Common-build-problems).

Now that you've got your version of Python installed, you can create a new
virtual environment with this command:

pyenv virtualenv 3.7.0 salt

Then activate it:

pyenv activate salt

Sweet! Now you're ready to clone salt so you can start hacking away!

waynew marked this conversation as resolved.
Show resolved Hide resolved
### Cloning Salt

waynew marked this conversation as resolved.
Show resolved Hide resolved
Clones are so shallow. Well, this one is anyway:

git clone --depth=1 --origin salt https://github.com/saltstack/salt.git

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 commits as you go.
If you *really* want all 108,300+ commits you can just run `git fetch
--unshallow`. Then go make a sandwich because it's gonna be a while.

### Fork Salt

You're also going to want to head over to GitHub and create your own [fork of
salt](https://github.com/saltstack/salt/fork). Once you've got that setup you
can add it as a remote:
waynew marked this conversation as resolved.
Show resolved Hide resolved

git remote add yourname <YOUR SALT REMOTE>

If you use your name to refer to your fork, and `salt` to refer to the official
Salt repo you'll never get `upstream` or `origin` confused.

### Install sphinx
waynew marked this conversation as resolved.
Show resolved Hide resolved

In order to build the docs, you'll need to install sphinx into your virtual
environment:

python -m pip install sphinx
waynew marked this conversation as resolved.
Show resolved Hide resolved

We'll use this later.

### `pre-commit` Setup

Here at Salt we use [pre-commit](https://pre-commit.com/) to make it easier for
contributors to get quick feedback. It configures some Git pre-commit hooks to
run `black` for formatting, `isort` for keeping our imports sorted, and
`pylint` to catch issues like unused imports, among others. You can easily
install it in your virtualenv with:

python -m pip install pre-commit
pre-commit init
waynew marked this conversation as resolved.
Show resolved Hide resolved

Now before each commit, it will ensure that your code at least *looks* right
before you open a pull request. And with that step, it's time to start hacking
on Salt!


## Selecting an Issue

waynew marked this conversation as resolved.
Show resolved Hide resolved
If you don't already have an issue in mind, you can search for [help
wanted](https://github.com/saltstack/salt/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22)
issues. If you also search for [good first
issue](https://github.com/saltstack/salt/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22+label%3A%22good+first+issue%22)
then you should be able to find some issues that are good for getting started
contributing to Salt. [Documentation
issues](https://github.com/saltstack/salt/issues?q=is%3Aissue+is%3Aopen+label%3Adocumentation+)
are also good starter issues. When you find an issue that catches your eye (or
one of your own), it's a good idea to comment on the issue and mention that
you're working on it. Good communication is key to collaboration - so if you
don't have time to complete work on the issue, just leaving some information
about when you expect to pick things up again is a great idea!

## Hacking Away

### Salt, Tests, Documentation, and You

To merge code contributions, Salt requires:

- documentation
- meaningful passing tests
- correct code

waynew marked this conversation as resolved.
Show resolved Hide resolved
Documentation fixes just require correct documentation.

#### What If I Don't Write Tests or Docs?

waynew marked this conversation as resolved.
Show resolved Hide resolved
If you aren't into writing documentation or tests, we still welcome your
contributions! But your PR will be labeled `Needs Testcase` and `Help Wanted`
until someone can get to write the tests/documentation. Of course, if you have
a desire but just lack the skill we are more than happy to collaborate and help
out! There's the [documentation working
group](https://github.com/saltstack/docs-hub) and the [testing working
group](https://github.com/saltstack/community/tree/master/working_groups/wg-Testing).
We also regularly stream our test clinic [live on
Twitch](https://www.twitch.tv/saltstackinc) every Tuesday afternoon and
Thursday morning, Central Time. If you'd like specific help with tests, bring
them to the clinic. If no community members need help, you can also just watch
tests written in real time.

### Documentation

Salt uses both docstrings, as well as normal reStructuredText files in the
`salt/doc` folder for documentation. To build the docs and view them in your
browser, simply run:

cd doc; make html; cd _build/html; python -m http.server
waynew marked this conversation as resolved.
Show resolved Hide resolved
waynew marked this conversation as resolved.
Show resolved Hide resolved

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 you can go to
http://localhost:8000/contents.html and view the docs as they exist. If you
make changes, you can simply run this:

cd -; make html; cd _build/html; python -m http.server

And get your updated docs.

If your change is a doc-only change, you can go ahead and commit/push your code
and open a PR. Otherwise you'll want to write some tests and code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: How can they mark that it's a docs-only change, perhaps in their MR title?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll call that out explicitly 👍


### Running Development Salt

Note: If you run into any issues in this section, check the Troubleshooting
section.

If you're going to hack on the Salt codebase you're going to want to be able to
run Salt locally. The first thing you need to do is install Salt as an editable
pip install:

python -m pip install -e .

This will let you make changes to Salt without having to re-install it.

After all of the dependencies and Salt are installed, it's time to set up the
config for development. Typically Salt runs as `root`, but you can specify
which user to run as. To configure that, just copy the master and minion
configs. We have .gitignore setup to ignore the `local/` directory, so we can
put all of our personal files there.

mkdir -p local/etc/salt/

Create a master config file as `local/etc/salt/master`:

cat <<EOF >local/etc/salt/master
user: $(whoami)
root_dir: $PWD/local/
publish_port: 55505
ret_port: 55506
EOF

And a minion config as `local/etc/salt/minion`:

cat <<EOF >local/etc/salt/minion
user: $(whoami)
root_dir: $PWD/local/
master: localhost
id: saltdev
master_port: 55506
EOF

Now you can start your Salt master and minion, specifying the config dir.

salt-master --config-dir=local/etc/salt/ --log-level=debug --daemon
salt-minion --config-dir=local/etc/salt/ --log-level=debug --daemon

Now you should be able to accept the minion key:

salt-key -c local/etc/salt -Ay

And check that your master/minion are communicating:

salt -c local/etc/salt \* test.version

Rather than running `test.version` from your master, you can run it from the
minion instead:

salt-call -c local/etc/salt test.version

Note that you're running `salt-call` instead of `salt`, and you're not
specifying the minion (`\*`), but if you're running the dev version then you
still will need to pass in the config dir. Now that you've got Salt running,
you can hack away on the Salt codebase!

waynew marked this conversation as resolved.
Show resolved Hide resolved
### Test First? Test Last? Test Meaningfully!

You can write tests first or tests last, as long as your tests are meaningful
and complete! *Typically* the best tests for Salt are going to be unit tests.
Testing is [a whole topic on its
own](https://docs.saltstack.com/en/master/topics/tutorials/writing_tests.html),
But you may also want to write functional or integration tests. You'll find
those in the `salt/tests` directory.

When you're thinking about tests to write, the most important thing to keep in
mind is, "What, exactly, am I testing?" When a test fails, you should know:

- What, specifically, failed?
- Why did it fail?
- As much as possible, what do I need to do to fix this failure?

If you can't answer those questions then you might need to refactor your tests.

When you're running tests locally, you should make sure that if you remove your
code changes your tests are failing. If your tests *aren't* failing when you
haven't yet made changes, then it's possible that you're testing the wrong
thing.

But whether you adhere to TDD/BDD, or you write your code first and your tests
last, ensure that your tests are meaningful.

#### Running Tests

We use `nox`, similar to `tox`, to run our tests. If you don't have nox
installed:

python -m pip install nox

Now you can run your tests:

nox -e "pytest-3.7(coverage=False)" -- tests/unit/cli/test_batch.py

It's a good idea to install [espeak](https://github.com/espeak-ng/espeak-ng) or
use `say` on Mac if you're running some long-running tests. You can do
something like this:

nox -e "pytest-3.7(coverage=False)" -- tests/unit/cli/test_batch.py; espeak "Tests done, woohoo!"

That way you don't have to keep monitoring the actual test run.

### Changelog and Commit!

When you write your commit message you should use imperative style. Do this:

> Add frobnosticate capability

Don't do this:

> Added frobnosticate capability

But that advice is backwards for the changelog. We follow the
[keepachangelog](https://keepachangelog.com/en/1.0.0/) approach for our
changelog, and use towncrier to generate it for each release. As a contributor,
all that means is that you need to add a file to the `salt/changelog`
directory, using the `<issue #>.<type>` format. For instanch, if you fixed
issue 123, you would do:

echo "Made sys.doc inform when no minions return" > changelog/123.fixed

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?

This will also help you out, because when you go to create the PR it will
automatically insert the body of your commit messages.

## PR Time!

Once you've done all your dev work and tested locally, you should check out our
[PR
guidelines](https://docs.saltstack.com/en/develop/topics/development/pull_requests.html).
After you read that page, it's time to [open a new
PR](https://github.com/saltstack/salt/compare). Fill out the PR template - you
should have updated or created any necessary docs, and written tests if you're
providing a code change. When you submit your PR, we have a suite of tests that
will run across different platforms. You will also get a reviewer assigned. If
your PR is submitted during the week you should be able to expect some kind of
communication within that business day. If your tests are 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][salt on freenode], or Community Slack.

If, as mentioned earlier, you're not interested in writing tests or docs, just
note that in your PR. Something like, "I'm not interested in writing
docs/tests, I just wanted to provide this fix." Otherwise, we will request that
you add appropriate docs/tests.

Congrats! You're a Salt developer! You rock!

## Troubleshooting

### zmq.core.error.ZMQError

Once the minion starts, you may see an error like the following::

zmq.core.error.ZMQError: ipc path "/path/to/your/virtualenv/var/run/salt/minion/minion_event_7824dcbcfd7a8f6755939af70b96249f_pub.ipc" is longer than 107 characters (sizeof(sockaddr_un.sun_path)).

This means that the path to the socket the minion is using is too long. This is
a system limitation, so the only workaround is to reduce the length of this
path. This can be done in a couple different ways:

1. Create your virtualenv in a path that is short enough.
2. Edit the :conf_minion:`sock_dir` minion config variable and reduce its
length. Remember that this path is relative to the value you set in
:conf_minion:`root_dir`.

NOTE: The socket path is limited to 107 characters on Solaris and Linux, and
103 characters on BSD-based systems.

### No permissions to access ...

If you forget to pass your config path to any of the `salt*` commands, you might see

No permissions to access "/var/log/salt/master", are you running as the
correct user?

Just pass `-c local/etc/salt` (or whatever you named it)

### File descriptor limit

You might need to raise your file descriptor limit. You can check it with:

ulimit -n

If the value is less than 2047, you should increase it with:

ulimit -n 2047
# For c-shell:
limit descriptors 2047

[salt on freenode]: https://webchat.freenode.net/#salt