Pull Requests¶

Salt is a large software project with many developers working together. We encourage all Salt users to contribute new features, bug fixes and documentation fixes. For those who haven't contributed to a large software project before we encourage you to consider the following questions when preparing a pull request.

This isn't an exhaustive list and these aren't necessarily hard and fast rules, but these are things we consider when reviewing a pull request.

• Does this change work on all platforms? In cases where it does not, is an appropriate and easy-to-understand reason presented to the user? Is it documented as-such? Have we thought about all the possible ways this code might be used and accounted as best we can for them?

• Will this code work on versions of all Python we support? Will it work on future versions?

• Are Python reserved keywords used? Are variables named in a way that will make it easy for the next person to understand what's going on?

• Does this code present a security risk in any way? What is the worst possible thing that an attacker could do with this code? If dangerous cases are possible, is it appropriate to document them? If so, has this been done? Would this change pass muster with a professional security audit? Is it obvious to a person using this code what the risks are?

• Is it readable? Does it conform to our style guide? Is the code documented such that the next person who comes along will be able to read and understand it? Most especially, are edge-cases documented to avoid regressions? Will it be immediately evident to the next person who comes along why this change was made?

• If appropriate, has the person who wrote the code which is being modified been notified and included in the process?

• What are the performance implications of this change? Is there a more efficient way to structure the logic and if so, does making the change balance itself against readability in a sensible way? Do the performance characteristics of the code change based on the way it is being invoked (i.e., through an API or various command-line tools.) Will it be easy to profile this change if it might be a problem?

• Are caveats considered and documented in the change?

• Will the code scale? More critically, will it scale in both directions? Salt runs in data-centers and on Raspberry Pi installations in the Sahara. It needs to work on big servers and tiny devices.

• Is appropriate documentation written both in public-facing docs and in-line? How will the user know how to use this? What will they do if it doesn't work as expected? Is this something a new user will understand? Can a user know all they need to about this functionality by reading the public docs?

• Is this a change in behavior? If so, is it in the appropriate branch? Are deprecation warnings necessary? Have those changes been fully documented? Have we fully thought through what implications a change in behavior might have?

• How has the code been tested? If appropriate are there automated tests which cover this? Is it likely to regress? If so, how has the potential of that regression been mitigated? What is the plan for ensuring that this code works going forward?

• If it's asynchronous code, what is the potential for a race condition?

• Is this code an original work? If it's borrowed from another project or found online are the appropriate licensing/attribution considerations handled?

• Is the reason for the change fully explained in the PR? If not for review, this is necessary so that somebody in the future can go back and figure out why it was necessary.

• Is the intended behavior of the change clear? How will that behavior be known to future contributors and to users?

• Does this code handle errors in a reasonable way? Have we gone back through the stack as much as possible to make sure that an error cannot be raised that we do not account for? Are errors tested for as well as proper functionality?

• If the code relies on external libraries, do we properly handle old versions of them? Do we require a specific version and if so is this version check implemented? Is the library available on the same platforms that module in question claims to support? If the code was written and tested against a particular library, have we documented that fact?

• Can this code freeze/hang/crash a running daemon? Can it stall a state run? Are there infinite loops? Are appropriate timeouts implemented?

• Is the function interface well documented? If argument types can not be inferred by introspection, are they documented?

• Are resources such as file-handles cleaned-up after they are used?

• Is it possible that a reference-cycle exists between objects that will leak memory?

• Has the code been linted and does it pass all tests?

• Does the change fully address the problem or is it limited to a small surface area? By this, I mean that it should be clear that the submitter has looked for other cases in the function or module where the given case might also be addressed. If additional changes are necessary are they documented in the code as a FIXME or the PR and in Github as an issue to be tracked?

• Will the code throw errors/warnings/stacktraces to the console during normal operation?

• Has all the debugging been removed?

• Does the code log any sensitive data? Does it show sensitive data in process lists? Does it store sensitive data to disk and if so, does it do so in a secure manner? Are there potential race conditions in between writing the data to disk and setting the appropriate permissions?

• Is it clear from the solution that the problem is well-understood? How can somebody who has never seen the problem feel confident that this proposed change is the best one?

• What's hard-coded that might not need to be? Are we making sensible decisions for the user and allowing them to tune and change things where appropriate?

• Are utility functions used where appropriate? Does this change re-implement something we already have code for?

• Is the right thing being fixed? There are cases where it's appropriate to fix a test and cases where it's appropriate to fix the code that's under test. Which is best for the user? Is this change a shortcut or a solution that will be solid in the months and years to come?

• How will this code react to changes elsewhere in the code base? What is it coupled to and have we fully thought through how best to present a coherent interface to consumers of a given function or method?

• Does this PR try to fix too many bugs/problems at once?

• Should this be split into multiple PRs to make them easier to test and reason about?

Pull Request Requirements¶

The following outlines what is required before a pull request can be merged into the salt project. For each of these requirements, an exception can be made that requires 3 approvals before merge. The exceptions are detailed more below.