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

Provide more clarification in error message #1280

Merged
merged 2 commits into from
Aug 17, 2023
Merged

Conversation

mitchnegus
Copy link
Contributor

This provides more information via the error message when a copy/update operation fails due to potentially unsafe actions. It suggests that a user may want to use the --trust flag for templates that they trust.

Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @mitchnegus! 🙏

You'll need to update the error message assertions in tests/test_unsafe.py to make the test suite pass. Other than that, it looks good. 👍

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Please fix the tests. Thanks!

copier/errors.py Outdated Show resolved Hide resolved
@yajo yajo linked an issue Aug 3, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #1280 (5b2724f) into master (cc49bdc) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1280      +/-   ##
==========================================
- Coverage   97.01%   96.96%   -0.05%     
==========================================
  Files          48       48              
  Lines        4053     4053              
==========================================
- Hits         3932     3930       -2     
- Misses        121      123       +2     
Flag Coverage Δ
unittests 96.96% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
copier/errors.py 91.11% <100.00%> (ø)
tests/test_unsafe.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@mitchnegus
Copy link
Contributor Author

Sorry, I was away from my computer for a week and just got back to checking (didn't notice that this hadn't passed before).

All the tests should be fixed now.

Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

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

LGTM, @mitchnegus! 👍 Would you mind applying @yajo's requested change still? I'll merge afterwards.

@mitchnegus
Copy link
Contributor Author

LGTM, @mitchnegus! 👍 Would you mind applying @yajo's requested change still? I'll merge afterwards.

All set—I thought he was just referencing the broken tests and missed the period he had suggested. Should be good to go :)

@sisp
Copy link
Member

sisp commented Aug 10, 2023

Perfect 👌, thanks again! 🙇

@sisp sisp enabled auto-merge (squash) August 10, 2023 16:05
@sisp
Copy link
Member

sisp commented Aug 10, 2023

Ah, it seems @yajo still needs to approve as he requested that one change.

@sisp sisp disabled auto-merge August 10, 2023 16:07
@yajo yajo merged commit 931961f into copier-org:master Aug 17, 2023
16 of 17 checks passed
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.

Add a bit more info for unsafe features
3 participants