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

Improve Plan Process #243

Merged
merged 8 commits into from
Aug 18, 2022
Merged

Conversation

andrew-welker
Copy link
Contributor

I'm submitting a

  • bug fix
  • [x ] feature
  • documentation addition

What is the current behaviour?

Currently, errors resulting from files not being found are not bubbled up and factored in to execution of steps

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem

What is the expected behavior?

Missing files should be reported when necessary, and affect planning of steps

What is the motivation / use case for changing the behavior?

Better UX when files necessary for a step are missing or weren't created in a previous step

Please tell us about your environment:

Version (comtrya --version):
Operating system: Windows 11/WSL2 Ubuntu 20.04

@andrew-welker
Copy link
Contributor Author

@icepuma As you can see, I started with updating the resolve function to have the correct signature, then updated the Action trait's plan function to have a Result<Vec<Step>,anyhow::Error> signature...which obviously has far-reaching implications. I'm guessing this is the correct route to go down, but not sure.

@@ -210,7 +211,7 @@ mod tests {
..Default::default()
};

let steps = file_link_action.plan(&manifest, &contexts);
let steps = file_link_action.plan(&manifest, &contexts).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use a ? instead of the .unwrap() here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that since this one is a test, .unwrap() is a better option...if it's an error, it'll panic and fail the test anyway.

Errors that happen in the plan step will now be logged,
and panics shouldn't happen when an action fails to plan.
@codecov-commenter
Copy link

Codecov Report

Merging #243 (bdf4c1e) into main (8ce6e12) will decrease coverage by 0.01%.
The diff coverage is 14.81%.

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
- Coverage   45.65%   45.63%   -0.02%     
==========================================
  Files          72       72              
  Lines        2510     2511       +1     
==========================================
  Hits         1146     1146              
- Misses       1364     1365       +1     
Impacted Files Coverage Δ
app/src/commands/apply.rs 0.00% <0.00%> (ø)
lib/src/actions/binary/github.rs 0.00% <0.00%> (ø)
lib/src/actions/command/run.rs 23.07% <0.00%> (ø)
lib/src/actions/directory/copy.rs 33.33% <0.00%> (ø)
lib/src/actions/directory/create.rs 53.33% <0.00%> (ø)
lib/src/actions/file/copy.rs 15.25% <0.00%> (+0.73%) ⬆️
lib/src/actions/file/download.rs 26.66% <0.00%> (ø)
lib/src/actions/git/clone.rs 0.00% <0.00%> (ø)
lib/src/actions/macos/default.rs 0.00% <0.00%> (ø)
lib/src/actions/mod.rs 20.89% <0.00%> (+0.60%) ⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andrew-welker andrew-welker marked this pull request as ready for review August 15, 2022 01:20
@martintc
Copy link
Contributor

I get a test failing in the test suite when I run it against this branch. I don't get a repro on the main branch.

test atoms::http::download::tests::it_can ... FAILED

failures:

---- atoms::http::download::tests::it_can stdout ----
thread 'atoms::http::download::tests::it_can' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :

Does our github actions not run the test suite?

@martintc martintc merged commit d64e6f1 into comtrya:main Aug 18, 2022
@martintc martintc mentioned this pull request Aug 18, 2022
3 tasks
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

4 participants