Skip to content
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

docs(bpf): update unprivileged_bpf_disabled description #23378

Merged
merged 1 commit into from Feb 1, 2023

Conversation

spacewander
Copy link
Contributor

See https://www.suse.com/support/kb/doc/?id=000020545
Signed-off-by: spacewander spacewanderlzx@gmail.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

@spacewander spacewander requested review from a team as code owners January 26, 2023 11:25
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 26, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 26, 2023
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Jan 27, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 27, 2023
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That's a useful precision.

I've got a few observations:

Documentation/bpf/architecture.rst Outdated Show resolved Hide resolved
Documentation/bpf/architecture.rst Outdated Show resolved Hide resolved
@spacewander
Copy link
Contributor Author

Thanks! That's a useful precision.

I've got a few observations:

* Could you please improve the title of your second commit (or alternatively, squash them)?

* I see the SUSE ticket, but better link references for the commit description would be instead:
  
  * [git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=08389d888287c3823f80b0216766b71e17f0aba5](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=08389d888287c3823f80b0216766b71e17f0aba5)
  * [kernel.org/doc/html/v6.1/admin-guide/sysctl/kernel.html?highlight=unprivileged_bpf_disabled#unprivileged-bpf-disabled](https://www.kernel.org/doc/html/v6.1/admin-guide/sysctl/kernel.html?highlight=unprivileged_bpf_disabled#unprivileged-bpf-disabled)

* Please find some additional feedback inline below.

Thanks! Updated.

@aanm aanm requested a review from qmonnet January 29, 2023 22:35
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good, thank you!

(Please next time squash the commits when it makes no sense to keep the second separate - I did it this time.)

@spacewander
Copy link
Contributor Author

Looks all good, thank you!

(Please next time squash the commits when it makes no sense to keep the second separate - I did it this time.)

Thanks! I will do it the next time.

BTW, we can use GitHub's "Squash and merge" button on the PR page to squash the commits automatically before merging. The Redefine GitHub extension has the ability to remove the intermediate commit message when squashing via web UI: https://github.com/refined-github/refined-github#editing-pull-requests. I recommend these because they have saved me lots of time when I maintain open source projects. 😃

See:
git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=08389d888287c3823f80b0216766b71e17f0aba5
kernel.org/doc/html/v6.1/admin-guide/sysctl/kernel.html?highlight=unprivileged_bpf_disabled#unprivileged-bpf-disabled

[ Quentin: squashed second commit (table right-side extension). ]

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@spacewander
Copy link
Contributor Author

Just sync the master branch into this PR...
It seems this project requires the PR to be up-to-date.

@qmonnet
Copy link
Member

qmonnet commented Feb 1, 2023

BTW, we can use GitHub's "Squash and merge" button

We have it disabled on Cilium. Although the feature could be useful at times, we prefer 1) avoiding the risk of squashing by accident when we do need to keep distinct commits to understand the changes, 2) avoiding the risks of confusion for users who are not so familiar with Git and that would see their branch modified under them, 3) avoiding to put the burden of choosing between rebase or squash for the person who merges (as a consequence, the responsibility to split/squash appropriately usually falls down to the commit author).

It seems this project requires the PR to be up-to-date.

No it's fine, as long as there's no conflict you can ignore this warning.

@qmonnet
Copy link
Member

qmonnet commented Feb 1, 2023

Checkpatch and documentation workflow have passed. Travis error is #23314. This PR is only related to docs, and the reviews are in, so we're good to go.

@qmonnet qmonnet merged commit 6784418 into cilium:master Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants