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

Adds rosdep-skip-keys option. #753

Conversation

francocipollone
Copy link
Contributor

Resolves #276

Summary

Adds an input option for passing the keys that are intended to be skipped during rosdep install command

Expected to be use as

    - uses: ros-tooling/action-ros-ci@ref
      with:
        ...
        rosdep-skip-keys: <key-to-skip-1> <key-to-skip-2> ... <key-to-skip-n>

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone francocipollone requested a review from a team as a code owner June 2, 2022 15:30
@francocipollone francocipollone requested review from gbiggs and christophebedard and removed request for a team June 2, 2022 15:30
@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #753 (44aef1e) into master (2db8455) will increase coverage by 0.56%.
The diff coverage is 80.00%.

❗ Current head 44aef1e differs from pull request most recent head 673ce05. Consider uploading reports for the commit 673ce05 to get more accurate results

@@            Coverage Diff             @@
##           master     #753      +/-   ##
==========================================
+ Coverage   49.61%   50.18%   +0.56%     
==========================================
  Files           2        2              
  Lines         262      267       +5     
  Branches       68       71       +3     
==========================================
+ Hits          130      134       +4     
- Misses        132      133       +1     
Impacted Files Coverage Δ
src/action-ros-ci.ts 44.11% <80.00%> (+0.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2db8455...673ce05. Read the comment docs.

@christophebedard
Copy link
Member

Thanks for the PR! Can you think of a way to add a test? For example, you could add a test case in .github/workflows/test.yml that uses this new option for a set of packages, then you verify that a skipped dependency wasn't installed (and maybe check that it is installed otherwise). Doesn't need to be a complicated test.

@francocipollone
Copy link
Contributor Author

Thanks for the PR! Can you think of a way to add a test? For example, you could add a test case in .github/workflows/test.yml that uses this new option for a set of packages, then you verify that a skipped dependency wasn't installed (and maybe check that it is installed otherwise). Doesn't need to be a complicated test.

Thanks for the suggestion.
I have only one doubt, when I skip a key, that dependency won't be installed, therefore when building the package it will fail because there are missing dependencies (If the package is correctly set-up that should happen :D)

And if it fails then the test will fail. How can I avoid it to fail so I can verify later on that the dependency isn't installed?

@christophebedard
Copy link
Member

I have only one doubt, when I skip a key, that dependency won't be installed, therefore when building the package it will fail because there are missing dependencies (If the package is correctly set-up that should happen :D)

And if it fails then the test will fail. How can I avoid it to fail so I can verify later on that the dependency isn't installed?

If this was always the case, this rosdep-skip-keys option would not really be useful 😛 What's your use-case for adding this option?

You could find a repo that has a <depend>ency which isn't really strictly needed, create a .repos file with that repo, add it to this repo, and use it in a test in .github/workflows/test.yml.

If you're not sure how to do it, I can add a test later, after this PR.

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone
Copy link
Contributor Author

What's your use-case for adding this option?

Haha good point! In our case we are bringing a cmake package (non-colcon package) to the workspace

You could find a repo that has a ency which isn't really strictly needed, create a .repos file with that repo, add it to this repo, and use it in a test in .github/workflows/test.yml.

I ended reusing one of the repos file used for other test. I am just skipping a test dependency, not building the test and then verifying that effectively the package isn't installed.

Please check 673ce05

@christophebedard
Copy link
Member

I ended reusing one of the repos file used for other test. I am just skipping a test dependency, not building the test and then verifying that effectively the package isn't installed.

That's perfect! Thank you :)

Now I'll just let CI finish.

@christophebedard
Copy link
Member

Thanks for the new feature!

@christophebedard christophebedard merged commit 8d7f7c6 into ros-tooling:master Jun 3, 2022
@francocipollone
Copy link
Contributor Author

Thanks @christophebedard for the immediate release 🚀

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.

Allow for specifying rosdep keys to skip
2 participants