-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix env parsing by placing it before executable parsing #81
Conversation
Signed-off-by: Nate Koenig <[email protected]>
I think it makes sense, because it is the expected behavior here. |
This adds a simple check to verify that the env directives apply to everything in the ignition namespace, as fixed in #81. The test itself is a bit of a workaround. We don't have a mechanism for checking the manager output, and executable command directives cannot expand environment variables. This writes a script that is executed by the test to verify that a file specified by an environment variable is touch-ed Signed-off-by: Michael Carroll <[email protected]>
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 have a proposed unit test in #82 if you want to take a look.
This adds a simple check to verify that the env directives apply to everything in the ignition namespace, as fixed in #81. The test itself is a bit of a workaround. We don't have a mechanism for checking the manager output, and executable command directives cannot expand environment variables. This writes a script that is executed by the test to verify that a file specified by an environment variable is touch-ed Signed-off-by: Michael Carroll <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-launch2 #81 +/- ##
===============================================
+ Coverage 29.01% 30.24% +1.22%
===============================================
Files 3 3
Lines 734 734
===============================================
+ Hits 213 222 +9
+ Misses 521 512 -9
Continue to review full report at Codecov.
|
* Fix env parsing by placing it before executable parsing Signed-off-by: Nate Koenig <[email protected]> * Add tests to #81 (#82) This adds a simple check to verify that the env directives apply to everything in the ignition namespace, as fixed in #81. The test itself is a bit of a workaround. We don't have a mechanism for checking the manager output, and executable command directives cannot expand environment variables. This writes a script that is executed by the test to verify that a file specified by an environment variable is touch-ed Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Michael Carroll <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Michael Carroll <[email protected]>
* Fix env parsing by placing it before executable parsing (#81) * Fix env parsing by placing it before executable parsing Signed-off-by: Nate Koenig <[email protected]> * Add tests to #81 (#82) This adds a simple check to verify that the env directives apply to everything in the ignition namespace, as fixed in #81. The test itself is a bit of a workaround. We don't have a mechanism for checking the manager output, and executable command directives cannot expand environment variables. This writes a script that is executed by the test to verify that a file specified by an environment variable is touch-ed Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Michael Carroll <[email protected]> * Prep for 2.2.1 release (#84) Signed-off-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Nate Koenig <[email protected]> Co-authored-by: Michael Carroll <[email protected]>
The application of environment variables in launch files was broken due to incorrect order of operations. Environment variables need to be set before forking processes. This fixes the problem.
Example:
In the above example, the
GazeboServer
would haveIGN_TRANSPORT_TOPIC_STATISTICS=1
butGazeboGui
would haveIGN_TRANSPORT_TOPIC_STATISTICS = undefined
.This is a behavior change from the currently release code, but this fix is fairly important.
Signed-off-by: Nate Koenig [email protected]