-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
devguide: discuss backports and other updates - v1 #9884
Changes from all commits
4dfb0d6
a3985c2
2004cb0
e21aace
3a6615b
f7695ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
======================== | ||
Suricata Backports Guide | ||
======================== | ||
|
||
This document describes the processes used to backport content to previous | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/previous/current stable ? |
||
Suricata releases. Most often, this means security and/or bug fixes; | ||
however, in some cases, features may be backported to previous Suricata releases. | ||
|
||
There are multiple versions of Suricata at any given time: | ||
* Master | ||
* Current release | ||
* Release prior to current release | ||
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about the words here. Perhaps something like
|
||
|
||
For example, at the moment, there are 3 releases based on these Suricata branches: | ||
* master: 8.0.0-dev, current development branch | ||
* main-7.0.x: current stable release (note we're changing our naming conventions) | ||
* master-6.0.x: previous stable release | ||
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we plan on upgrading this doc everytime we EOL a branch or create a new one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. Considering it's something to be done every... two or three years, I don't think it's a bad idea to review/ check for needed updates at least then. imho. |
||
|
||
The next sections discuss when and what to backport, and some guidelines when | ||
doing so. | ||
|
||
What should be backported? | ||
-------------------------- | ||
|
||
Usually, when the team creates a ticket, we'll add the *Needs backport* related | ||
labels, so necessary backporting tickets will be automatically created. If you | ||
are working on a ticket that doesn't have such labels, nor backporting tasks | ||
associated, it probably doesn't need backporting. But sometimes we'll miss those. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or if they think it's something that should be backported, they should comment on the ticket. |
||
|
||
There can be cases where backports may be "missed" -- some issues may not be | ||
labeled as needing backports and some PRs may be merged without an issue. | ||
Comment on lines
+30
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps, these should go in an Exceptions or something section at the end? Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add a note in this section, I think it's easier to keep context, like that. 🤔 |
||
|
||
The general principle used to determine what will be backported is: | ||
* evasion/security fixes | ||
* bug fixes | ||
* in some cases, new features are backported if there are sufficient reasons to | ||
backport a new feature. | ||
|
||
This guide may be insufficient for some situations. When in doubt, please reach | ||
out to the team on the backport ticket or PR. | ||
|
||
Selection overview | ||
------------------ | ||
|
||
All items considered for backports should be reviewed with the following: | ||
* risk estimate: will the change introduce new bugs? Consider the scope and | ||
items affected by the change. | ||
* behavioral change: how much will the behavior of the system be changed by the | ||
backport. for example, a small change to decode additional encapsulation | ||
protocols may result in more traffic being presented to Suricata. | ||
* default settings: if the issue alters behavior, can it be made optional, and | ||
at what cost? | ||
|
||
Creating backport tickets -- new issues | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
Redmine: when creating a new redmine issue, label the Redmine issue with "needs | ||
backport to x.y.z”, where x.y.x is a supported Suricata release, e.g, 7.0.x, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Needs backport to x.0" |
||
for security/evasion and bug fixes. | ||
|
||
Creating backports tickets -- existing issues/PRs | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
We want to minimize the occurrence of "missed backports" -- that is, work that | ||
should be backported but wasn't. sometimes this happens when there is no Redmine | ||
issue, or the redmine issue wasn't labeled as needing a backport. | ||
|
||
Therefore, we will be periodically reviewing: | ||
* redmine issues without backport labels, including recently closed issues, to | ||
see which require backport labels. | ||
* prs without associated redmine issues. Those requiring backports should be | ||
labeled with *needs backport*. | ||
|
||
Then, also periodically, we will create backport issues from those items | ||
identified in the previous steps. When doing so, we will evaluate what are the | ||
relevant target backport releases. Some issues reported against master or the | ||
current Suricata release may not apply to older releases. | ||
|
||
Git Backport Workflow | ||
--------------------- | ||
|
||
* *Identify the commit(s) needed* for the backport. Start with the PR that merged | ||
the commits into master and select only the commits from the issue being | ||
backported. | ||
* *Bring each commit into the new branch,* one at a time -- starting with the oldest | ||
commit. Use ``git cherry-pick -x commit-hash`` as it maintains the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add something like "where commit-hash is the commit from master that is being backported" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch, thanks!! |
||
linkage with the cherry-picked commit. | ||
* *Resolve conflicts:* Some of the cherry-picked commits may contain merge | ||
conflicts. If the conflicts are small, include the corrections in the | ||
cherry-picked commit. | ||
* *Add additional commits*, if any are needed (e.g., to adjust cherry-picked code | ||
to old behavior). | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its not often, but sometimes something was rewritten in the latest version but the old version still needs to be fixed. In this case there are no commits to backport. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if this belongs to this section, or to the part about creating backport tickets. Do I understand this correctly that this is about, for instance, something that has changed from 6 to 7, and was now fixed in 8, so there could be a backport for 7, but not for 6? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No. I'm thinking more about a bug that is found to affect 7 and git master, but where cherry-pick ID's don't really make sense. A good example would be the case where something was rewritten in Rust. In master you'd be doing a patch in Rust, and in 7 you'd be doing a patch in C. Refactors, etc. can also result in this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thanks, got it, I'll see how to incorporate that. |
||
Create a PR: | ||
~~~~~~~~~~~~ | ||
|
||
Please indicate in the title that this is a backport PR, with something like | ||
*(7.0.x-backport)*. | ||
|
||
In the PR description, indicate the backport ticket. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we, should we use the milestone feature in GitHub? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that! |
||
QA | ||
-- | ||
|
||
Add suricata-verify PRs when needed. Some existing suricata-verify tests may require | ||
version specification changes. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
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...) | ||
|
||
The 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 | ||
|
||
Once submitted, we aim at providing a first PR review within two weeks and a | ||
month. | ||
|
||
If either code, documentation wording or commit messages need re-work, the | ||
reviewer will set the PR state to *changes requested*. | ||
|
||
.. note:: It is expected that the author will create a new PR with a new version | ||
of the patch as described in :ref:`Pull Requests Criteria <pull-requests-criteria>`. | ||
A PR may be closed as stale if it has not been updated in two months after | ||
changes were requested. | ||
|
||
A PR may be labeled *decision-required* if the reviewer thinks the team needs | ||
more time to analyze the best approach to a proposed solution or discussion | ||
raised by the PR. | ||
|
||
Once in approved state, the PRs are in the responsibility of the merger, along | ||
with the next branches/PRs. | ||
|
||
Reviewers and Maintainers | ||
------------------------- | ||
|
||
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, it should get a review status within 2 weeks: | ||
either changes requested, approved, or assigned to someone else if more | ||
expertise is needed. | ||
|
||
The GitHub filter for changes-requested PRs is:: | ||
|
||
is:pr is:open draft:false sort: updated-asc review:changes-requested | ||
|
||
The command to get approved PRs is:: | ||
|
||
gh pr list --json number,reviewDecision --search "state:open type:pr -review:none" | jq '.[] | select(.reviewDecision=="")' | ||
|
||
An approved PR should match the filter: ``is:open is:pr review:approved``. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,4 @@ Contributing | |
contribution-process | ||
code-submission-process | ||
github-pr-workflow | ||
backports-guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the hash part of this URL.