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 ROS environment variables to options #700

Merged

Conversation

kenji-miyake
Copy link
Contributor

@kenji-miyake kenji-miyake commented Aug 11, 2021

Refinement of #697

In order to support dependencies with conditions like <depend condition="$ROS_VERSION == 2">, I added ROS_VERSION and ROS_PYTHON_VERSION in install_rosdeps.sh.

The followings are my test results.

Current Implementation This PR
1. Docker Image kenji-miyake/action-ros-ci-test-condition#1 ✔️ kenji-miyake/action-ros-ci-test-condition#16
2. setup-ros kenji-miyake/action-ros-ci-test-condition#6 ✔️ kenji-miyake/action-ros-ci-test-condition#17
3. Source Build
(condition-dependency isn't in ros2.repos)
kenji-miyake/action-ros-ci-test-condition#11 ✔️ kenji-miyake/action-ros-ci-test-condition#18
4. Source Build
(condition-dependency is in ros2.repos)
kenji-miyake/action-ros-ci-test-condition#20 ✔️ kenji-miyake/action-ros-ci-test-condition#21

The difference between 3 and 4 is whether the environment variables are required in the rosdep install step. 3 requires and 4 doesn't.

Note: In some PRs, rolling test is failed, but it's not related to this PR.

@kenji-miyake kenji-miyake requested a review from a team as a code owner August 11, 2021 16:05
@kenji-miyake kenji-miyake requested review from christophebedard and zmichaels11 and removed request for a team August 11, 2021 16:05
@kenji-miyake kenji-miyake marked this pull request as draft August 11, 2021 16:09
@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #700 (be7e99e) into master (7ff80b8) will decrease coverage by 1.41%.
The diff coverage is 0.00%.

❗ Current head be7e99e differs from pull request most recent head 934b760. Consider uploading reports for the commit 934b760 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
- Coverage   49.38%   47.96%   -1.42%     
==========================================
  Files           2        2              
  Lines         243      246       +3     
  Branches       53       57       +4     
==========================================
- Hits          120      118       -2     
- Misses        123      128       +5     
Impacted Files Coverage Δ
src/action-ros-ci.ts 41.01% <0.00%> (-1.51%) ⬇️

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 7ff80b8...934b760. Read the comment docs.

src/action-ros-ci.ts Outdated Show resolved Hide resolved
Kenji Miyake added 2 commits August 14, 2021 03:58
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
@kenji-miyake kenji-miyake changed the title Define environment variables for rosdep Add ROS environment variables to options Aug 13, 2021
@kenji-miyake kenji-miyake marked this pull request as ready for review August 13, 2021 19:41
Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

This LGTM - nice patch

@christophebedard christophebedard added the enhancement New feature or request label Aug 14, 2021
Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>

Run npm commands

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
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.

This is a great enhancement! Thank you very much for iterating @kenji-miyake!

@christophebedard christophebedard enabled auto-merge (squash) August 14, 2021 15:02
@christophebedard christophebedard merged commit c69b164 into ros-tooling:master Aug 14, 2021
@kenji-miyake kenji-miyake deleted the define-env-for-rosdep branch August 14, 2021 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants