-
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
Conversation
This could be justified from a semantic point of view, and also can help in bringing more attention to where this information is, as it is less hidden, now.
This section seemed to aim both at PR reviewers and PR authors at the same time, even though some info is probably of low value for contributors. Created new section for PR reviewers and maintainers, and kept the info for PR authors separated. Also highlighted information on requested changes and stale PRs.
If a commit introduces code that changes Suricata behavior, we understand that the documentation changes should go in the same commit. Among other things, this reduces the chances of said changes being lost if there are backports or many work versions before approval.
Update the list of active branches to include 7 renaming and new master, link to backports document.
Sphinx and RtD sometimes render lists in weird ways. The communication channels list barely looked like one, at all...
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 comment
The 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 comment
The 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 comment
The 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?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks, got it, I'll see how to incorporate that.
*(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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I like that!
@@ -13,14 +13,15 @@ developed by the [OISF](https://oisf.net) and the Suricata community. | |||
- [Home Page](https://suricata.io) | |||
- [Bug Tracker](https://redmine.openinfosecfoundation.org/projects/suricata) | |||
- [User Guide](https://docs.suricata.io) | |||
- [Dev Guide](https://docs.suricata.io/en/latest/devguide/index.html#suricata-developer-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.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9884 +/- ##
==========================================
- Coverage 82.45% 82.45% -0.01%
==========================================
Files 973 973
Lines 273063 273063
==========================================
- Hits 225155 225143 -12
- Misses 47908 47920 +12
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Backports doc looking good from a brief overview. :)
Left a few suggestions inline and nit fixes.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
s/previous/current stable ?
* Current release | ||
* Release prior to current release |
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.
Not sure about the words here. Perhaps something like
- master
- major stable 1 ...
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 |
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.
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 comment
The 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.
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 comment
The 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. |
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.
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 comment
The 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. 🤔
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
"Needs backport to x.0"
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thanks!!
Feedback incorporated on: #9914 |
Link to redmine ticket:
Describe changes:
working with the codebase
chapter.-
or*
seems to impact how they are rendered?Result can be seen at: https://suri-rtd-test.readthedocs.io/en/contribution-docs-updates-v1/devguide/contributing/backports-guide.html