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

Add skip-rosdep-install & rosdep-check options #830

Merged
merged 6 commits into from
Jan 24, 2024

Conversation

kaichie
Copy link
Contributor

@kaichie kaichie commented Jul 30, 2023

Closes #829

This PR adds an optional argument for skipping rosdep install in the action (skip-rosdep-install) and an optional argument to run rosdep check (rosdep-check), which only works if skip-rosdep-install is also enabled.

@kaichie kaichie requested a review from a team as a code owner July 30, 2023 16:16
@kaichie kaichie requested review from gbiggs and MichaelOrlov and removed request for a team July 30, 2023 16:16
@christophebedard
Copy link
Member

Could you just sign-off your commit? git commit --amend --no-edit -s and then force push.

@christophebedard christophebedard changed the title skip rosdep install Add skip-rosdep-install option Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (bc41bd3) 47.53% compared to head (63520d1) 45.81%.

Files Patch % Lines
src/action-ros-ci.ts 12.50% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
- Coverage   47.53%   45.81%   -1.72%     
==========================================
  Files           2        2              
  Lines         284      299      +15     
  Branches       77       80       +3     
==========================================
+ Hits          135      137       +2     
- Misses        149      162      +13     

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

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

So sorry for the long delay!

I have a suggestion and some requests:

  1. Can you add a new section in the README to document the skip-rosdep-install and rosdep-check options? Like the sections here: https://github.com/ros-tooling/action-ros-ci#Usage. You could add a single section for both options, since they're tightly connected. It would be good to explicitly mention why users might want to use these options, e.g., to fail if dependencies are not properly declared.
  2. If possible, a simple test that uses these 2 options (and doesn't fail) would be good.

Also, please rebase on master.

action.yml Outdated Show resolved Hide resolved
@roncapat
Copy link

@kaichie may I help advance this PR somehow? I'm not good in writing tests in this context, but I may help with the docs for example.

Signed-off-by: nelson <kaichie.lee@gmail.com>
Signed-off-by: nelson <kaichie.lee@gmail.com>
Signed-off-by: nelson <kaichie.lee@gmail.com>
Signed-off-by: nelson <kaichie.lee@gmail.com>
Signed-off-by: nelson <kaichie.lee@gmail.com>
@kaichie
Copy link
Contributor Author

kaichie commented Dec 28, 2023

@christophebedard Added the suggestions and requested changes, let me know if I missed anything. Thanks for checking!

@roncapat Appreciate your offer to assist! I have updated the test and the docs. However, feel free to make any adjustment to the docs if you think it's appropriate. 😃

@roncapat
Copy link

@christophebedard friendly ping :)

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

A few more suggestions! Then this should be ready.

README.md Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: nelson <kaichie.lee@gmail.com>
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@christophebedard christophebedard changed the title Add skip-rosdep-install option Add skip-rosdep-install & rosdep-check options Jan 24, 2024
@christophebedard christophebedard merged commit d3afb09 into ros-tooling:master Jan 24, 2024
27 of 34 checks passed
@christophebedard
Copy link
Member

I released this as 0.3.6/v0.3 https://github.com/ros-tooling/action-ros-ci/releases/tag/0.3.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip rosdep install
3 participants