-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Expand condition_script_runner_args
#1132
Expand condition_script_runner_args
#1132
Conversation
@wmmc88 thanks for the PR! would love if you see my comment |
…_args` since it relies on a non-released rust-script change
@sagiegurari I've updated the PR so that there is no change in env when condition evaluation fails (and added a test to prevent possible future regressions) |
@sagiegurari friendly ping to review this :) the underlying bug is a blocker for me |
@wmmc88 sorry for the delay! i'll go over it today |
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.
@wmmc88 everything looks pretty great just 2 main thinigs
- docs - the file is generated so your changes will be deleted. lets try not to do the nav changes you did but only the content. you can add nav, but not add a new root please.
- env setup before the condition and restoring it causes new env vars defined in the task to stick even if condition fails. we need to remove them.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1132 +/- ##
===========================================
- Coverage 92.81% 69.01% -23.80%
===========================================
Files 119 119
Lines 26224 26347 +123
===========================================
- Hits 24339 18183 -6156
- Misses 1885 8164 +6279 ☔ View full report in Codecov by Sentry. |
@sagiegurari I think I've resolved all your requested changes |
Thanks a lot @wmmc88 for the PR. merged. |
Hi @sagiegurari! friendly bump for a release |
@wmmc88 sorry for really late handling of this but 0.37.16 is now released with this fix. thanks for the pr |
#1081 added
condition_script_runner_args
, butcondition_script_runner_args
didn't expand env vars likescript_runner_args
did. This PR adds env expansion tocondition_script_runner_args
, and also adds tests for it.Various bugfixes to tests were also fixed in order for
cargo make ci-flow
to pass on:CARGO_HOME