-
Notifications
You must be signed in to change notification settings - Fork 745
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
added --allowoverwrite flag to decompile #6029
Conversation
Looks like a test needs fixing or updating. |
updated the test. |
Thanks! Sending the bug through triage to verify the exact behavior we want (see #5659 (comment)). I think your PR is the right behavior, but it is a breaking change. |
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.
Requested some changes. Also awaiting decision from triage on desired behavior.
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.
Looking great! Only one remaining item... We need testcases for the new functionality. I'd suggested adding the following two scenarios to. DecompileCommandTests.cs:
Decompile_OutputFileExists_ShouldFail
Decompile_OutputFileExists_Force_ShouldSuccess
Again, let me know if you'd like help or don't have time. Thanks!
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.
Thanks!
Fixes: #5659
Contributing a Pull Request
If you haven't already, read the full contribution guide. The guide may have changed since the last time you read it, so please double-check. Once you are done and ready to submit your PR, run through the relevant checklist below.
dotnet test
for bicep CLI unit tests