diff options
author | Daniel Baumann <daniel.baumann@progress-linux.org> | 2024-04-19 17:39:49 +0000 |
---|---|---|
committer | Daniel Baumann <daniel.baumann@progress-linux.org> | 2024-04-19 17:39:49 +0000 |
commit | a0aa2307322cd47bbf416810ac0292925e03be87 (patch) | |
tree | 37076262a026c4b48c8a0e84f44ff9187556ca35 /doc/userguide/devguide/codebase/contributing | |
parent | Initial commit. (diff) | |
download | suricata-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')
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 |