summaryrefslogtreecommitdiffstats
path: root/doc/userguide/devguide/codebase/contributing
diff options
context:
space:
mode:
authorDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-19 17:39:49 +0000
committerDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-19 17:39:49 +0000
commita0aa2307322cd47bbf416810ac0292925e03be87 (patch)
tree37076262a026c4b48c8a0e84f44ff9187556ca35 /doc/userguide/devguide/codebase/contributing
parentInitial commit. (diff)
downloadsuricata-a0aa2307322cd47bbf416810ac0292925e03be87.tar.xz
suricata-a0aa2307322cd47bbf416810ac0292925e03be87.zip
Adding upstream version 1:7.0.3.upstream/1%7.0.3
Signed-off-by: Daniel Baumann <daniel.baumann@progress-linux.org>
Diffstat (limited to 'doc/userguide/devguide/codebase/contributing')
-rw-r--r--doc/userguide/devguide/codebase/contributing/code-submission-process.rst68
-rw-r--r--doc/userguide/devguide/codebase/contributing/contribution-process.rst271
-rw-r--r--doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst46
-rw-r--r--doc/userguide/devguide/codebase/contributing/index.rst9
4 files changed, 394 insertions, 0 deletions
diff --git a/doc/userguide/devguide/codebase/contributing/code-submission-process.rst b/doc/userguide/devguide/codebase/contributing/code-submission-process.rst
new file mode 100644
index 0000000..22bf160
--- /dev/null
+++ b/doc/userguide/devguide/codebase/contributing/code-submission-process.rst
@@ -0,0 +1,68 @@
+Code Submission Process
+=======================
+
+.. _commits:
+
+Commits
+~~~~~~~
+
+#. Commits need to be logically separated. Don't fix unrelated things in one commit.
+#. Don't add unnecessary commits, if commit 2 fixes commit 1 merge them together (squash)
+#. Commits need to have proper messages, explaining anything that is non-trivial
+#. Commits should not at the same time change, rename and/or move code. Use separate commits
+ for each of this, e.g, a commit to rename files, then a commit to change the code.
+#. Documentation updates should be in their own commit (not mixed with code commits)
+#. Commit messages need to be properly formatted:
+ * Meaningful and short (50 chars max) subject line followed by an empty line
+ * Naming convention: prefix message with sub-system ("rule parsing: fixing foobar"). If
+ you're not sure what to use, look at past commits to the file(s) in your PR.
+ * Description, wrapped at ~72 characters
+#. Commits should be individually compilable, starting with the oldest commit. Make sure that
+ each commit can be built if it and the preceding commits in the PR are used.
+#. Commits should be authored with the format: "FirstName LastName <name@example.com>"
+
+Information that needs to be part of a commit (if applicable):
+
+#. Ticket it fixes. E.g. "Fixes Bug #123."
+#. Compiler warnings addressed.
+#. Coverity Scan issues addressed.
+#. Static analyzer error it fixes (cppcheck/scan-build/etc)
+
+When in doubt, check our git history for other messages or changes done to the
+same module your're working on. This is a good example of a `commit message
+<https://github.com/OISF/suricata/commit/33fca4d4db112b75ffa22eb2e6f14f038cbcc1f9>`_::
+
+ pcap/file: normalize file timestamps
+
+ Normalize the timestamps that are too far in the past to epoch.
+
+ Bug: #6240.
+
+.. _pull-requests-criteria:
+
+Pull Requests
+~~~~~~~~~~~~~
+
+A github pull request is actually just a pointer to a branch in your tree. GitHub provides a review interface that we use.
+
+#. A branch can only be used in for an individual PR.
+#. A branch should not be updated after the pull request
+#. A pull request always needs a good description (link to issue tracker if related to a ticket).
+#. Incremental pull requests need to link to the prior iteration
+#. Incremental pull requests need to describe changes since the last PR
+#. Link to the ticket(s) that are addressed to it.
+#. When fixing an issue, update the issue status to ``In Review`` after submitting the PR.
+#. Pull requests are automatically tested using github actions (https://github.com/OISF/suricata/blob/master/.github/workflows/builds.yml).
+ Failing builds won't be considered and should be closed immediately.
+#. Pull requests that change, or add a feature should include a documentation update commit
+
+Tests and QA
+~~~~~~~~~~~~
+
+As much as possible, new functionality should be easy to QA.
+
+#. Add ``suricata-verify`` tests for verification. See https://github.com/OISF/suricata-verify
+#. Add unittests if a ``suricata-verify`` test isn't possible.
+#. Provide pcaps that reproduce the problem. Try to trim as much as possible to the pcap includes the minimal
+ set of packets that demonstrate the problem.
+#. Provide example rules if the code added new keywords or new options to existing keywords
diff --git a/doc/userguide/devguide/codebase/contributing/contribution-process.rst b/doc/userguide/devguide/codebase/contributing/contribution-process.rst
new file mode 100644
index 0000000..198ff14
--- /dev/null
+++ b/doc/userguide/devguide/codebase/contributing/contribution-process.rst
@@ -0,0 +1,271 @@
+************************
+Contributing to Suricata
+************************
+
+This guide describes what steps to take if you want to contribute a patch or
+patchset to Suricata.
+
+Essentially, these are:
+
+#. Agree to and sign our :ref:`Contribution Agreement<contribution-agreement>`
+#. Communicate early, and use the :ref:`preferred channels <communication-channels>`
+#. :ref:`claim-ticket`
+#. :ref:`Fork from master <what-branch-to-work-on>`
+#. Follow our :ref:`Coding Style`
+#. Use our :ref:`documentation-style`
+#. Stick to our :ref:`commit guidelines<commits>`
+#. Add version numbers to your :ref:`Pull Requests <send-a-pull-request>`
+#. Incorporate :ref:`feedback` into new PRs
+#. [Work merged] :ref:`Wrap up! <wrap-up>`
+
+The rest of this document will cover those in detail.
+
+.. _contribution-agreement:
+
+.. note:: Important!
+
+ Before contributing, please review and sign our `Contribution Agreement
+ <https://suricata.io/contribution-agreements/>`_.
+
+.. _communication-channels:
+
+Communication is Key!
+=====================
+
+To clarify questions, discuss or suggest new features, talk about bugs and
+optimizations, and/or ask for help, it is important to communicate.
+
+These are our main channels:
+
+- `Suricata's issue tracker <https://redmine.openinfosecfoundation.org/
+ projects/suricata/issues>`_
+- `Suricata's forum <https://forum.suricata.io/c/developers/8>`_
+- `Suricata's Discord server <https://discord.com/invite/t3rV2x7MrG>`_
+
+
+.. _claim-ticket:
+
+Claim (or open) a ticket
+========================
+
+For features and bugs we need `tickets <https://redmine.openinfosecfoundation.
+org/projects/suricata/issues>`_. Tickets help us keep track of the work done,
+indicate when changes need backports etc.
+
+They are also important if you would like to see your new feature officially
+added to our tool: the ticket documents your ideas so we can analyze how do they
+fit in our plans for Suricata, and, if the feature is accepted, we can properly
+track progress etc.
+
+.. note:: If you want to add new functionalities (e.g. a new application layer
+ protocol), please ask us first whether we see that being merged into
+ Suricata or not. This helps both sides understand how the new feature will
+ fit in our roadmap, and prevents wasting time and motivation with
+ contributions that we may not accept. Therefore, `before` starting any code
+ related to a new feature, do request comments from the team about it.
+
+For really trivial fixes or cleanups we won't need that.
+
+Once work on the issue has been agreed upon:
+
+Assign the ticket to yourself. For this, you will need to have the "developer"
+role. You can ask for that directly on the ticket you want to claim or mention
+that you are interested in working on `ticket number` on our `Developer's
+channel on Discord <https://discord.com/channels/864648830553292840/
+888087709002891324>`_.
+
+If a ticket is already assigned to someone, please reach out on the ticket or
+ask the person first.
+
+You can reach out to other community members via `Suricata's Discord server
+<https://discord.com/invite/t3rV2x7MrG>`_.
+
+
+Expectations
+============
+
+If you submit a new feature that is not part of Suricata's core functionalities,
+it will have the `community supported`_ status. This means we would expect some
+commitment from you, or the organization who is sponsoring your work, before we
+could approve the new feature, as the Suricata development team is pretty lean
+(and many times overworked).
+
+This means we expect that:
+
+ * the new contribution comes with a set of Suricata-verify tests (and
+ possibly unit tests, where those apply), before we can approve it;
+ * proof of compatibility with existing keywords/features is provided,
+ when the contribution is for replacing an existing feature;
+ * you would maintain the feature once it is approved - or some other
+ community member would do that, in case you cannot.
+
+.. note::
+
+ Regardless of contribution size or complexity, we expect that you respect
+ our guidelines and processes. We appreciate community contributors:
+ Suricata wouldn't be what it is without them; and the value of our tool and
+ community also comes from how seriously we take all this, so we ask that
+ our contributors do the same!
+
+.. _community supported:
+
+What does "community supported" and "supporting a feature" mean?
+-----------------------------------------------------------------
+
+If a feature is *community supported*, the Suricata team will try to spend
+minimal time on it - to be able to focus on the core functionalities. If for any
+reason you're not willing or able to commit to supporting a feature, please
+indicate this.
+
+The team and/or community members can then consider offering help. It is best
+to indicate this prior to doing the actual work, because we will reject features
+if no one steps up.
+
+It is also important to note that *community supported* features will be
+disabled by default, and if it brings in new dependencies (libraries or Rust
+crates) those will also be optional and disabled by default.
+
+**Supporting a feature** means to actually *maintain* it:
+
+* fixing bugs
+* writing documentation
+* keeping it up to date
+* offering end-user support via forum and/or Discord chat
+
+.. _stale-tickets-policy:
+
+Stale tickets policy
+====================
+
+We understand that people's availability and interested to volunteer their time
+to our project may change. Therefore, to prevent tickets going stale (not worked
+on), and issues going unsolved for a long time, we have a policy to unclaim
+tickets if there are no contribution updates within 6 months.
+
+If you claim a ticket and later on find out that you won't be able to work on
+it, it is also appreciated if you inform that to us in the ticket and unclaim
+it, so everyone knows that work is still open and waiting to be done.
+
+.. _what-branch-to-work-on:
+
+What branch to work on
+======================
+
+There are 2 or 3 active branches:
+
+ * master-x.x.x (e.g. master-6.x.y)
+ * master
+
+The former is the stable branch. The latter the development branch.
+
+The stable branch should only be worked on for important bug fixes. Those are
+mainly expected from more experienced contributors.
+
+Development of new features or large scale redesign is done in the development
+branch. New development and new contributors should work with ``master`` except
+in very special cases - which should and would be discussed with us first.
+
+If in doubt, please reach out to us via :ref:`Redmine, Discord or
+forum <communication-channels>`.
+
+.. _create-your-own-branch:
+
+Create your own branch
+======================
+
+It's useful to create descriptive branch names. You're working on ticket 123 to
+improve GeoIP? Name your branch "geoip-feature-123-v1". The "-v1" addition is
+for feedback. When incorporating feedback you will have to create a new branch
+for each pull request. So, when you address the first feedback, you will work in
+"geoip-feature-123-v2" and so on.
+
+For more details check: `Creating a branch to do your changes <https://redmine.
+openinfosecfoundation.org/projects/suricata/wiki/GitHub_work_flow#Creating-a-
+branch-to-do-your-changes>`_
+
+
+Coding Style
+============
+
+We have a :ref:`Coding Style` that must be followed.
+
+.. _documentation-style:
+
+Documentation Style
+===================
+
+For documenting *code*, please follow Rust documentation and/or Doxygen
+guidelines, according to what your contribution is using (Rust or C).
+
+If you are writing or updating *documentation pages*, please:
+
+* wrap up lines at 79 (80 at most) characters;
+* when adding diagrams or images, we prefer alternatives that can be generated
+ automatically, if possible;
+* bear in mind that our documentation is published on `Read the Docs <https:/
+ /docs.suricata.io/en/latest/#suricata-user-guide>`_ and can also be
+ built to pdf, so it is important that it looks good in such formats.
+
+
+Commit History matters
+======================
+
+Please consider our :ref:`Commit guidelines <commits>` before submitting your PR.
+
+.. _send-a-pull-request:
+
+Send a Pull Request
+===================
+
+The pull request is used to request inclusion of your patches into the main
+repository. Before it is merged, it will be reviewed and pushed through a QA
+process.
+
+Please consider our :ref:`Pull Requests Criteria <pull-requests-criteria>` when
+submitting.
+
+We have 'GitHub-CI' integration enabled. This means some automated build check,
+suricata-verity and unit tests are performed on the pull request. Generally,
+this is ready after a few minutes. If the test fails, the pull request won't be
+considered. So please, when you submit something, keep an eye on the checks,
+and address any failures - if you do not understand what they are, it is fine to
+ask about them on the failing PR itself.
+
+Before merge, we also perform other integration tests in our private QA-lab. If
+those fail, we may request further changes, even if the GitHub-CI has passed.
+
+.. _feedback:
+
+Feedback
+========
+
+You'll likely get some feedback. Even our most experienced devs do, so don't
+feel bad about it.
+
+After discussing what needs to be changed (usually on the PR itself), it's time
+to go back to ":ref:`create-your-own-branch`" and do it all again. This process
+can iterate quite a few times, as the contribution is refined.
+
+.. _wrap-up:
+
+Wrapping up
+===========
+
+Merged! Cleanup
+---------------
+
+Congrats! Your change has been merged into the main repository. Many thanks!
+
+We strongly suggest cleaning up: delete your related branches, both locally and
+on GitHub - this helps you in keeping things organized when you want to make new
+contributions.
+
+.. _update-ticket:
+
+Update ticket
+-------------
+
+You can now put the URL of the *merged* pull request in the Redmine ticket.
+Next, mark the ticket as "Closed" or "Resolved".
+
+Well done! You are all set now.
diff --git a/doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst b/doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst
new file mode 100644
index 0000000..618c966
--- /dev/null
+++ b/doc/userguide/devguide/codebase/contributing/github-pr-workflow.rst
@@ -0,0 +1,46 @@
+GitHub Pull Request Workflow
+============================
+
+Draft Pull Requests
+~~~~~~~~~~~~~~~~~~~
+
+A Pull Request (PR) should be marked as `draft` if it is not intended to be merged as is,
+but is waiting for some sort of feedback.
+The author of the PR should be explicit with what kind of feedback is expected
+(CI/QA run, discussion on the code, etc...)
+
+GitHub filter is ``is:pr is:open draft:true sort:updated-asc``
+
+A draft may be closed if it has not been updated in two months.
+
+Mergeable Pull Requests
+~~~~~~~~~~~~~~~~~~~~~~~
+
+When a Pull Request is intended to be merged as is, the workflow is the following:
+ 1. get reviewed, and either request changes or get approved
+ 2. if approved, get staged in a next branch (with other PRs), wait for CI validation
+ (and eventually request changes if CI finds anything)
+ 3. get merged and closed
+
+A newly created PR should match the filter
+``is:pr is:open draft:false review:none sort:updated-asc no:assignee``
+The whole team is responsible to assign a PR to someone precise within 2 weeks.
+
+When someone gets assigned a PR, the PR should get a review status within 2 weeks:
+either changes requested, approved, or assigned to someone else if more
+expertise is needed.
+
+GitHub filter for changes-requested PRs is ``is:pr is:open draft:false sort:
+updated-asc review:changes-requested``
+
+Such a PR may be closed if it has not been updated in two months.
+It is expected that the author creates a new PR with a new version of the patch
+as described in :ref:`Pull Requests Criteria <pull-requests-criteria>`.
+
+Command to get approved PRs is ``gh pr list --json number,reviewDecision --search
+"state:open type:pr -review:none" | jq '.[] | select(.reviewDecision=="")'``
+
+Web UI filter does not work cf https://github.com/orgs/community/discussions/55826
+
+Once in approved state, the PRs are in the responsibility of the merger, along
+with the next branches/PRs.
diff --git a/doc/userguide/devguide/codebase/contributing/index.rst b/doc/userguide/devguide/codebase/contributing/index.rst
new file mode 100644
index 0000000..e0d2912
--- /dev/null
+++ b/doc/userguide/devguide/codebase/contributing/index.rst
@@ -0,0 +1,9 @@
+Contributing
+============
+
+.. toctree::
+ :maxdepth: 2
+
+ contribution-process
+ code-submission-process
+ github-pr-workflow