[WIP] Introduce a coding style for `libs` repo #470

Andreagit97 · 2022-07-11T13:37:01Z

What type of PR is this?

/kind design

/kind feature

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:

This PR puts in place all the necessary stuff to enforce a definitive code style for our repo. On one side, we provide different simple ways to apply the new style before pushing patches upstream (Makefile, pre-commit framework), on the other, we use GitHub actions to check that the style is enforced before merging the code.

The new Contributing.md file explains all the new features added, so for more info please take a look at it.

Here you can find some additional points:

As @Molter73 suggested I added a way to enforce both the coding style and the DCO signed-off through the pre-commit framework, let me know if it looks good to you! 😄
You can find the configuration for clang-format and cmake-format tools in the corresponding configuration files .clang-format .cmake-format.json. I used mainly the default configuration for both the tools, plus some little modifications as @deepskyblue86 asked (I set the tabsize to 4, this seems reasonable to me, but let me know about it)
I merged the old coding_conventions.md file into the new Contributing.md. I kept only some of the best practices since the others are already addressed by the new code style.
I have added a new GitHub action to run static analysis code with clang-tidy. This check will run only on files changed by the PR and more precisely it notifies about issues regarding the only changed lines in this file. In case of detected issues, this action will directly comment on the pull request under the precise line involved in the issue. Please note: this action is not blocking the merging, it is just a suggestion... I don't know if we want this action right now, but it could be a good idea IMHO, let me know what you think about it :)
The clang-format and cmake-format jobs will provide you a git-diff to resolve all the style issues in just one shot

Which issue(s) this PR fixes:

Fixes #381

Special notes for your reviewer:

Probably we want to preserve git history before applying the new style to all the code, this is why the pull request is still in [WIP] status. We have to decide how to proceed here.

Does this PR introduce a user-facing change?:

NONE

* set column limit to `100` * SortInclude must be `Never` not `False` * Disable formatting for other languages, just to be sure Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

poiana · 2022-07-11T13:37:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Andreagit97]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Andreagit97 · 2022-07-11T13:49:46Z

There seem to be some problems with the permission for issuing comments on the PR, I will try to address them ASAP, by the way, they are not necessary at all, they are just a nitpick 😄

Molter73

This is awesome @Andreagit97! Thank you very much for implementing it!

Molter73 · 2022-07-11T13:43:45Z

.clang-tidy

@@ -0,0 +1,44 @@
+---


Yes, yes, 1000 times yes!

.cmake-format.json

Molter73 · 2022-07-11T13:59:00Z

.github/workflows/linting.yml

+      - name: Install clang-tools 📥
+        uses: KyleMayes/install-llvm-action@v1.5.3
+        with:
+          version: 12.0.0


Why are we using a different LLVM version for clang-tidy? 🤔
If we can use the same for both? Is there any way we can ensure we will always use the same version? Maybe it can be added as an environment variable to the repository and controlled globally from there?

Yeah, I know, unfortunately, the GitHub action that prints messages in the PR accepts only YAML files containing generated fixes by only some clang versions :( I used the latest so clang-12

.github/workflows/linting.yml

Molter73 · 2022-07-11T14:09:41Z

Makefile

@@ -0,0 +1,80 @@
+#


Since we are using pre-commit for checking commits, think it would be worth it to also use it to run the linters/formatters instead of defining custom was of running the tools? We can keep the make targets for ease of use, but I think unifying methods could help reduce potential incompatibilities on silly things like command line flags mismatching.

You can take this as an example if it helps: https://github.com/stackrox/collector/blob/843fbc328028ad6a6cfa0fb281f7f08703c40e65/Makefile#L171-L182

And I just realized you mention this in Contributing.md. I know it's tempting to allow people to choose whichever way the want to use hooks, but in my mind it is still better to unify it since it will likely lead to less problems due to compatibility in the long term. Would love to hear other people opinion on this.

That's a good point! Personally, I prefer to have both methods since we use this makefile also in the GitHub actions... using makefile is clearer we just type the exact command we want, I consider it as the real source of truth. I added the pre-commit framework to help people who don't want to install these tools with their hands but I don't like too much the approach since you have no full control as in makefile. Anyway, we can discuss that, maybe someone else has some opinion on this 🤔

I thought on it a little bit and we could also use the pre-commit framework in the CI, at the end it's almost the same thing 🤔 What do you think about it @leogr @deepskyblue86 @FedeDP?

Signed-off-by: Andrea Terzolo andrea.terzolo@polito.it Co-authored-by: Mauro Ezequiel Moltrasio <moltrasiom@hotmail.com>

deepskyblue86 · 2022-07-11T17:26:58Z

@Andreagit97 this is awesome, but I'll have the action blocking the merge, or else this could be just wasted effort.

nit: several files don't have the newline at the end.

poiana · 2023-09-05T19:33:29Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

Andreagit97 · 2023-09-06T08:34:08Z

/remove-lifecycle stale

poiana · 2023-12-05T09:46:58Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

Andreagit97 · 2023-12-05T10:32:45Z

/remove-lifecycle stale

poiana · 2024-03-04T15:49:31Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

incertum · 2024-03-04T21:15:46Z

/remove-lifecycle stale

poiana · 2024-06-02T21:53:24Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

Andreagit97 · 2024-06-03T08:06:35Z

/remove-lifecycle stale

leogr · 2024-06-04T08:06:16Z

I would like to bring this up again

Andreagit97 · 2024-08-22T14:16:58Z

if we all agree I would move this to 0.18.0

/milestone 0.18.0

FedeDP · 2024-08-22T15:55:45Z

I agree! We wait a bit long for this one to land 😄

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

poiana · 2024-08-27T15:16:19Z

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

github-actions · 2024-08-27T15:20:30Z

Perf diff from master - unit tests

     2.76%     +1.39%  [.] gzfile_read
     3.47%     +1.38%  [.] sinsp_evt::load_params
     5.76%     -1.16%  [.] sinsp_evt::get_type
     2.05%     +0.99%  [.] sinsp_thread_manager::find_thread
     4.48%     -0.83%  [.] sinsp_parser::process_event
     5.93%     -0.67%  [.] next
     7.10%     -0.63%  [.] sinsp::next
     2.10%     -0.53%  [.] sinsp::fetch_next_event
     0.22%     +0.50%  [.] sinsp_fdtable::find
     1.75%     +0.49%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

codecov · 2024-08-27T15:43:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.20%. Comparing base (eb374bc) to head (135cc56).
Report is 29 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #470   +/-   ##
=======================================
  Coverage   74.20%   74.20%           
=======================================
  Files         253      253           
  Lines       30832    30832           
  Branches     5402     5387   -15     
=======================================
  Hits        22880    22880           
+ Misses       7943     7924   -19     
- Partials        9       28   +19

Flag	Coverage Δ
libsinsp	`74.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

leogr · 2024-08-28T10:20:18Z

if we all agree I would move this to 0.18.0

/milestone 0.18.0

🥳

Andreagit97 · 2024-08-29T15:38:33Z

closed in favor of #2038

Andreagit97 added 10 commits July 11, 2022 11:10

chore(clang-format): change the configuration file:

80a73bf

* set column limit to `100` * SortInclude must be `Never` not `False` * Disable formatting for other languages, just to be sure Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

chore(cmake-format): scaffold default cmake-format conf file

f85053d

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

chore(cmake-format): bump line width to 100 as for clang-format

de7233f

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

chore(Makefile): add a Makefile to enforce new coding style

8ae8b48

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

chore(git_hooks): add a bash script to enforce DCO

770458a

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

chore(pre-commit): add the pre-commit configuration file

54eac33

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

docs: add new doc regarding coding style

736c322

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

chore: update some configuration files

a208a91

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

ci: add github actions to lint the code

16add0b

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

doc: update contributing documentation

f76234e

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

poiana added do-not-merge/work-in-progress release-note-none kind/design dco-signoff: yes kind/feature New feature or request area/build size/XXL labels Jul 11, 2022

poiana requested review from fntlnz and mstemm July 11, 2022 13:37

poiana added the approved label Jul 11, 2022

Molter73 reviewed Jul 11, 2022

View reviewed changes

poiana added dco-signoff: no and removed dco-signoff: yes labels Jul 11, 2022

Andreagit97 force-pushed the new_code_style branch from 3656845 to f76234e Compare July 11, 2022 15:21

poiana added dco-signoff: yes and removed dco-signoff: no labels Jul 11, 2022

doc: fix a typo

95323df

Signed-off-by: Andrea Terzolo andrea.terzolo@polito.it Co-authored-by: Mauro Ezequiel Moltrasio <moltrasiom@hotmail.com>

poiana added the lifecycle/stale label Sep 5, 2023

poiana removed the lifecycle/stale label Sep 6, 2023

poiana added the lifecycle/stale label Dec 5, 2023

poiana removed the lifecycle/stale label Dec 5, 2023

poiana added the lifecycle/stale label Mar 4, 2024

poiana removed the lifecycle/stale label Mar 4, 2024

incertum mentioned this pull request Mar 14, 2024

[tracking] Improve code documentation #1747

Open

poiana added the lifecycle/stale label Jun 2, 2024

poiana removed the lifecycle/stale label Jun 3, 2024

Andreagit97 modified the milestones: TBD, 0.18.0 Aug 22, 2024

Merge branch 'master' into new_code_style

135cc56

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

poiana added the do-not-merge/contains-merge-commits label Aug 27, 2024

Andreagit97 closed this Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Introduce a coding style for `libs` repo #470

[WIP] Introduce a coding style for `libs` repo #470

Andreagit97 commented Jul 11, 2022 •

edited

Loading

poiana commented Jul 11, 2022

Andreagit97 commented Jul 11, 2022

Molter73 left a comment

Molter73 Jul 11, 2022

Molter73 Jul 11, 2022

Andreagit97 Jul 11, 2022

Molter73 Jul 11, 2022

Molter73 Jul 11, 2022

Andreagit97 Jul 11, 2022 •

edited

Loading

Andreagit97 Jul 11, 2022

deepskyblue86 commented Jul 11, 2022

poiana commented Sep 5, 2023

Andreagit97 commented Sep 6, 2023

poiana commented Dec 5, 2023

Andreagit97 commented Dec 5, 2023

poiana commented Mar 4, 2024

incertum commented Mar 4, 2024

poiana commented Jun 2, 2024

Andreagit97 commented Jun 3, 2024

leogr commented Jun 4, 2024

Andreagit97 commented Aug 22, 2024

FedeDP commented Aug 22, 2024

poiana commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

codecov bot commented Aug 27, 2024 •

edited

Loading

leogr commented Aug 28, 2024

Andreagit97 commented Aug 29, 2024

[WIP] Introduce a coding style for libs repo #470

[WIP] Introduce a coding style for libs repo #470

Conversation

Andreagit97 commented Jul 11, 2022 • edited Loading

poiana commented Jul 11, 2022

Andreagit97 commented Jul 11, 2022

Molter73 left a comment

Choose a reason for hiding this comment

Molter73 Jul 11, 2022

Choose a reason for hiding this comment

Molter73 Jul 11, 2022

Choose a reason for hiding this comment

Andreagit97 Jul 11, 2022

Choose a reason for hiding this comment

Molter73 Jul 11, 2022

Choose a reason for hiding this comment

Molter73 Jul 11, 2022

Choose a reason for hiding this comment

Andreagit97 Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Andreagit97 Jul 11, 2022

Choose a reason for hiding this comment

deepskyblue86 commented Jul 11, 2022

poiana commented Sep 5, 2023

Andreagit97 commented Sep 6, 2023

poiana commented Dec 5, 2023

Andreagit97 commented Dec 5, 2023

poiana commented Mar 4, 2024

incertum commented Mar 4, 2024

poiana commented Jun 2, 2024

Andreagit97 commented Jun 3, 2024

leogr commented Jun 4, 2024

Andreagit97 commented Aug 22, 2024

FedeDP commented Aug 22, 2024

poiana commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

leogr commented Aug 28, 2024

Andreagit97 commented Aug 29, 2024

[WIP] Introduce a coding style for `libs` repo #470

[WIP] Introduce a coding style for `libs` repo #470

Andreagit97 commented Jul 11, 2022 •

edited

Loading

Andreagit97 Jul 11, 2022 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading