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

Allow for explicit workspace flow in sub flows #455

Merged
merged 1 commit into from Sep 6, 2020

Conversation

epontan
Copy link
Contributor

@epontan epontan commented Sep 2, 2020

Commit fa929b1 ("Fix workspace detection for sub flows") caused a
regression when having a root workspace with tasks that relies on each
member to define CARGO_MAKE_CARGO_BUILD_TEST_FLAGS with unique set of
features.

Determine if workspace flow is explicitly set in the task when planning
the execution and take this setting into account when running in a sub
flow.

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2020

Codecov Report

Merging #455 into 0.32.5 will increase coverage by 0.17%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.32.5     #455      +/-   ##
==========================================
+ Coverage   92.63%   92.80%   +0.17%     
==========================================
  Files          92       92              
  Lines       17886    17797      -89     
==========================================
- Hits        16568    16516      -52     
+ Misses       1318     1281      -37     
Impacted Files Coverage Δ
src/lib/execution_plan_test.rs 95.44% <94.59%> (+0.50%) ⬆️
src/lib/execution_plan.rs 93.80% <100.00%> (+1.99%) ⬆️
src/lib/config.rs 72.41% <0.00%> (-10.35%) ⬇️
src/lib/cache.rs 56.45% <0.00%> (-6.63%) ⬇️
src/lib/scriptengine/duck_script/mod_test.rs 91.66% <0.00%> (-2.78%) ⬇️
src/lib/version.rs 93.44% <0.00%> (-2.50%) ⬇️
src/lib/installer/crate_version_check.rs 73.19% <0.00%> (-1.56%) ⬇️
src/lib/environment/crateinfo.rs 83.33% <0.00%> (-1.56%) ⬇️
src/lib/functions/mod.rs 91.66% <0.00%> (-1.39%) ⬇️
src/lib/installer/rustup_component_installer.rs 71.23% <0.00%> (-0.77%) ⬇️
... and 23 more

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 87e590f...1176842. Read the comment docs.

@sagiegurari sagiegurari changed the base branch from master to 0.32.5 September 3, 2020 18:26
@sagiegurari
Copy link
Owner

@epontan thanks a lot for the PR, it looks good.
I rebased it on top a new dev branch. not sure why the build failed, but lets see if it passes now.

@epontan
Copy link
Contributor Author

epontan commented Sep 4, 2020

@sagiegurari thank you! It looks like all passed except the code coverage this time. I'm not sure why it fails. If I run coverage-tarpaulin locally the coverage is increased by half a point.

@sagiegurari
Copy link
Owner

ya, its due to rustfmt compilation broken on nightly. just pushed a workaround, let me restart the GH actions and see how it goes.

@sagiegurari
Copy link
Owner

@epontan can you pull 032.5 changes to your local branch? this will contain the rustfmt workaround so we can see coverage is ok

Commit fa929b1 ("Fix workspace detection for sub flows") caused a
regression when having a root workspace with tasks that relies on each
member to define CARGO_MAKE_CARGO_BUILD_TEST_FLAGS with unique set of
features.

Determine if workspace flow is explicitly set in the task when planning
the execution and take this setting into account when running in a sub
flow.
@epontan
Copy link
Contributor Author

epontan commented Sep 4, 2020

@sagiegurari that seemed to have done the trick! Thanks 👍

@sagiegurari
Copy link
Owner

@epontan this is really great fix. thanks a lot!!!
merging

@sagiegurari sagiegurari merged commit ef23b20 into sagiegurari:0.32.5 Sep 6, 2020
sagiegurari added a commit that referenced this pull request Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants