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

ros2 param dump silently overwrites existing file #625

Closed
clalancette opened this issue Apr 16, 2021 · 6 comments
Closed

ros2 param dump silently overwrites existing file #625

clalancette opened this issue Apr 16, 2021 · 6 comments
Labels
enhancement New feature or request more-information-needed Further information is required

Comments

@clalancette
Copy link
Contributor

Bug report

Required Info:

  • Operating System:
    • Ubuntu 20.04
  • Installation type:
    • Debian packages
  • Version or commit hash:
    • ros-rolling-ros2param 0.13.0-1
  • DDS implementation:
    • CycloneDDS
  • Client library (if applicable):
    • rclpy

Steps to reproduce issue

In terminal 1:

ros2 run turtlesim turtlesim_node

In terminal 2:

echo "foo" > turtlesim.yaml  # turtlesim.yaml now contains "foo"
ros2 param dump /turtlesim  # this overwrites the existing turtlesim.yaml, which may be surprising

Expected behavior

When ros2 param dump is choosing a filename, it should either error out if a file with the name it chose already exists, or it should keep choosing names until it finds one that has no conflict.

Actual behavior

ros2 param dump silently ovewrites the existing file.

@hidmic
Copy link
Contributor

hidmic commented Apr 16, 2021

Sometimes it is practical for it to just overwrite it. Perhaps making opt-in would be best.

@clalancette
Copy link
Contributor Author

Sometimes it is practical for it to just overwrite it. Perhaps making opt-in would be best.

Yeah, I'd be fine with a --force or --overwrite flag to cause it to always overwrite. I just think that the current behavior could lead to data loss, where I was storing something I actually wanted in turtlesim.yaml, and running ros2 param dump /turtlesim in the same directory just overwrites it.

@sloretz
Copy link
Contributor

sloretz commented Apr 19, 2021

What if the default were to write to stdout, with an option to write to a named file? Then overwriting is no big deal because the user had to give a filename to write to.

@clalancette
Copy link
Contributor Author

What if the default were to write to stdout, with an option to write to a named file? Then overwriting is no big deal because the user had to give a filename to write to.

That's a pretty good idea, I like it. This is also more Unix-y; you could always do ros2 param dump /turtlesim > turtlesim.yaml.

The downside is that this breaks compatibility with earlier releases. That is, in Foxy, if you do ros2 param dump /turtlesim, by default it writes to a file. If you then pass --print, it prints to stdout. If we were to make this change, then in Galactic ros2 param dump /turtlesim would print to stdout, and passing --write <fname> (or whatever) would write it to a file.

I don't know, how do people feel about a command-line break like that?

@audrow audrow added enhancement New feature or request more-information-needed Further information is required labels Apr 23, 2021
@clalancette clalancette changed the title ros2 param dump silently ovewrites existing file ros2 param dump silently overwrites existing file May 13, 2021
@sloretz
Copy link
Contributor

sloretz commented Jul 12, 2021

@clalancette ok to close now that #638 is merged?

@clalancette
Copy link
Contributor Author

Yes! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request more-information-needed Further information is required
Projects
None yet
Development

No branches or pull requests

4 participants