ros-controls / ros2_control_ci

This repository holds reusable workflows for CI of the ros2_control framework.
https://control.ros.org
Apache License 2.0
2 stars 1 forks source link

[CI] Composite action and `os_name` #11

Closed christophfroehlich closed 6 months ago

christophfroehlich commented 6 months ago

Both were tested with https://github.com/ros-controls/control_toolbox/pull/187

christophfroehlich commented 6 months ago

I'm wondering if we need this workflow at all.

The claim from https://github.com/ros-tooling/action-ros-lint is not relevant for us, because we don't use this as a pre-condition of the build jobs:

This command does not compile any code intentionally to make it as fast as possible (<2 min). The objective is to give contributors feedback about their change quickly. It may also be used to avoid wasting CI resources (no need to compile, and run the tests if it does not pass the linters).

What is the benefit compared to the pre-commit format job? All four linter are already in the pre-commit config ament_copyright https://github.com/ros-controls/ros2_control_ci/blob/aa866611a771ebc3d9db342854925d6e881f08a0/.pre-commit-config.yaml#L102-L110 ament_cmake https://github.com/ros-controls/ros2_control_ci/blob/aa866611a771ebc3d9db342854925d6e881f08a0/.pre-commit-config.yaml#L91-L100 ament_cpplint https://github.com/ros-controls/ros2_control_ci/blob/aa866611a771ebc3d9db342854925d6e881f08a0/.pre-commit-config.yaml#L79-L89 ament_cppcheck https://github.com/ros-controls/ros2_control_ci/blob/aa866611a771ebc3d9db342854925d6e881f08a0/.pre-commit-config.yaml#L69-L77

fmauch commented 6 months ago

What is the benefit compared to the pre-commit format job? All four linter are already in the pre-commit config ament_copyright

Yes, we asked ourselves the same question a while back and switched to a pre-commit only linting. Note though that in the current configs the ament linters are run during the commit stage only (probably because of the mentioned duplication?)

Removing the ros-lint workflow also makes maintaining things easier since now two lists of linters have to be updated and best kept in sync from my understanding?

So: +1 for removint the ci-lint job.

christophfroehlich commented 6 months ago

I don't know how to configure pre-commit, what does commit-stage mean in this context? The pre-commit workflow is called on PRs, this is all we want?

fmauch commented 6 months ago

In this sample:

https://github.com/ros-controls/ros2_control_ci/blob/aa866611a771ebc3d9db342854925d6e881f08a0/.pre-commit-config.yaml#L102-L110

stages: [commit] means that this only runs when the "command" invoking pre-commit is a commit action. In the pre-commit ci we do

- uses: pre-commit/action@v3.0.1
      with:
        extra_args: --all-files --hook-stage manual

So only checks that have the manual stage enabled will run. See https://pre-commit.com/#confining-hooks-to-run-at-certain-stages for details.

So, if we remove the ros-lint workflow we would have to update the pre-commit configs by removing that line (or extend it by the manual stage).

christophfroehlich commented 6 months ago

Ah, now I understand. This means that with the current setup, these 4 linters weren't called twice in the CI. (pre-commit output is rather verbose, I wasn't aware of that)

I'd remove the stages: [commit] and all ros-lint jobs from all repos.