libs icon indicating copy to clipboard operation
libs copied to clipboard

Published 20 hours ago • verified publisher icon falcosecurity

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

Open Andreagit97 opened this issue 2 years ago • 23 comments

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! :smile:

  • 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

Andreagit97 avatar Jul 11 '22 13:07 Andreagit97

[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:

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

poiana avatar Jul 11 '22 13:07 poiana

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 :smile:

Andreagit97 avatar Jul 11 '22 13:07 Andreagit97

@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.

deepskyblue86 avatar Jul 11 '22 17:07 deepskyblue86

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

That's a good point @deepskyblue86 these checks must become required only when we are ready to apply the new code style, as I wrote in the Special notes for your reviewer section we need to understand when we want to officially enable this new standard according to the git history preservetion...

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

Thank you I will fix them :)

Andreagit97 avatar Jul 11 '22 18:07 Andreagit97

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 smile

I have changed the type of event on which the lint workflow is triggered from pull_request to pull_request_target, as you can read here this event allows us also to comment on pull requests from forks.

Another possible solution could be to modify the permissions for the GITHUB_TOKEN in individual workflow files, as you can read here. I have no strong opinion on that, in the end, the concept is almost the same...

Having done this change, we need to merge this pull request in order to start using these new GH actions

Andreagit97 avatar Jul 11 '22 22:07 Andreagit97

We need to remember that before applying the style to all the repo we need to solve some strange inconsistencies that trigger a non-deterministic behavior formatting with clang-format-14

Andreagit97 avatar Jul 15 '22 13:07 Andreagit97

Any news? 😊

deepskyblue86 avatar Oct 09 '22 15:10 deepskyblue86

Any news?

We will work again on that after the release, we miss only the way to preserve the git history, if possible :) @leogr

Andreagit97 avatar Oct 10 '22 22:10 Andreagit97

Dropping a bomb :bomb: :boom: ...

How I do like it (diff-friendly and quite readable):

sinsp_usergroup_manager::sinsp_usergroup_manager(sinsp* inspector)
    : m_import_users(true)
    , m_last_flush_time_ns(0)
    , m_inspector(inspector)
#if defined(HAVE_PWD_H) || defined(HAVE_GRP_H)
    , m_host_root(m_inspector->get_host_root())
#endif
{
}

How current clang-format settings would format it:

sinsp_usergroup_manager::sinsp_usergroup_manager(sinsp *inspector):
	m_import_users(true),
	m_last_flush_time_ns(0),
	m_inspector(inspector)
#if defined(HAVE_PWD_H) || defined(HAVE_GRP_H)
	,
	m_host_root(m_inspector->get_host_root())
#endif
{
}

deepskyblue86 avatar Nov 02 '22 12:11 deepskyblue86

I apologize because I have not found the time yet for this. I want to ensure you it's on my task list :pray:

leogr avatar Nov 14 '22 09:11 leogr

/milestone 0.11.0

FedeDP avatar Dec 02 '22 13:12 FedeDP

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

poiana avatar Mar 20 '23 09:03 poiana

/remove-lifecycle stale

FedeDP avatar Mar 20 '23 10:03 FedeDP

/milestone 0.12.0

FedeDP avatar Apr 27 '23 09:04 FedeDP

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

poiana avatar Sep 05 '23 19:09 poiana

/remove-lifecycle stale

Andreagit97 avatar Sep 06 '23 08:09 Andreagit97

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

poiana avatar Dec 05 '23 09:12 poiana

/remove-lifecycle stale

Andreagit97 avatar Dec 05 '23 10:12 Andreagit97

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

poiana avatar Mar 04 '24 15:03 poiana

/remove-lifecycle stale

incertum avatar Mar 04 '24 21:03 incertum