-
Notifications
You must be signed in to change notification settings - Fork 142
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 support for not optional environment variable substitution #288
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I'm ok with this style of providing a non-optional version of this substitution. We cannot, however, back port it (which is ok) and we should add a note to the changelog for Eloquent when that's available.
Signed-off-by: ivanpauno <[email protected]>
Signed-off-by: ivanpauno <[email protected]>
fbc1fa5
to
444231b
Compare
Signed-off-by: ivanpauno <[email protected]>
CI failures are all unrelated. Same three failures on windows' today nightly job. |
) * Add support for non optional environment variable substitution Signed-off-by: ivanpauno <[email protected]> * Address per review comments Signed-off-by: ivanpauno <[email protected]> * Correct test in launch_xml Signed-off-by: ivanpauno <[email protected]>
Based on comment ros2/ros2_documentation#302 (comment).
This is not exactly what I first commented, but mimics what we where doing in
LaunchConfiguration
.I think it makes more sense.
launch/launch/launch/substitutions/launch_configuration.py
Lines 32 to 40 in 0f81d98
launch/launch/launch/substitutions/launch_configuration.py
Lines 67 to 76 in 0f81d98
This breaks API. If someone was relying on
default_value
being''
, not it will raise aSubstitutionError
.The fix is passing
default_value=''
explicitly.