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.

Sign up for GitHub

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

Jump to bottom

If sources list file has default content, exit gracefully #771

Closed
wants to merge 3 commits into from
Closed

If sources list file has default content, exit gracefully #771

wants to merge 3 commits into from

Conversation

rotu
Copy link

@rotu rotu commented Jul 23, 2020

@rotu rotu changed the title Don't throw an error if sources list file has default content If sources list file has default content, exit gracefully Jul 23, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2020 •
edited

Codecov Report

Merging #771 into master will decrease coverage by 0.14%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
- Coverage   74.85%   74.71%   -0.15%     
==========================================
  Files          41       41              
  Lines        3214     3231      +17     
==========================================
+ Hits         2406     2414       +8     
- Misses        808      817       +9
Impacted Files Coverage Δ
src/rosdep2/main.py 48.59% <28.57%> (-0.25%) ⬇️
src/rosdep2/platforms/debian.py 79.24% <0.00%> (-0.50%) ⬇️

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 d1784da...b5c1572. Read the comment docs.

emersonknapp
emersonknapp approved these changes Sep 4, 2020
View reviewed changes
Copy link

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

Nice!

@rotu
Copy link
Author

rotu commented Sep 10, 2020

What’s my next step to get this merged in?

@emersonknapp
Copy link

Maybe we ping @nuclearsandwich @tfoote for review?

@nuclearsandwich
Copy link
Contributor

My initial reaction was that invoking rosdep init in a script like

test -f /etc/ros/rosdep/sources.list.d/20-default.list || rosdep init

would be a reasonable way to get this feature without a code change but that script assumes the default path would never change and doesn't warn if the contents update so this has obvious advantages.

@rotu would you be interested in adding tests for the two reinitialization cases? (same content and changed content)

nuclearsandwich
nuclearsandwich requested changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

I think all this needs is tests and then we can merge it.

@rotu
Copy link
Author

rotu commented Jun 25, 2021

@nuclearsandwich Feel free to take over this PR

@rotu rotu closed this Jun 25, 2021
@nuclearsandwich
Copy link
Contributor

@nuclearsandwich Feel free to take over this PR

Thanks, I'm not sure I'll get to it but if anyone wants to see this change merged open a PR with these changes and some tests and I'll review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuclearsandwich nuclearsandwich nuclearsandwich requested changes

@emersonknapp emersonknapp emersonknapp approved these changes

Assignees
No one assigned
Labels
awaiting feedback or changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error on attempt to rerun rosdep init in osrf containers
4 participants
@rotu @codecov-commenter @emersonknapp @nuclearsandwich