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

Make Capistrano respect --dry-run in run_locally #2133

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

edspc
Copy link

@edspc edspc commented May 31, 2023

Summary

As there is no solution for issue #1822, I suggest updating run_locally to respect dry_run and adding run_locally! to ignore it and keep original behaviour.

Based on @tycooon answer.

Short checklist

  • Did you run bundle exec rubocop -a to fix linter issues?
  • If relevant, did you create a test?
  • Did you confirm that the RSpec tests pass?

Other Information

I need this to use a custom SCM plugin that manages the git repository locally and creates a tar archive also locally. So during dry_run, I don't need to create an archive, but some git commands still should be executed.

@edspc edspc changed the title Make Capistrano respect dry_run in run_locally. Fixes [capistrano#1822] Make Capistrano respect dry_run in run_locally. Fixes #1822 May 31, 2023
@edspc edspc changed the title Make Capistrano respect dry_run in run_locally. Fixes #1822 Make Capistrano respect dry_run in run_locally May 31, 2023
@mattbrictson
Copy link
Member

@edspc thanks for the PR!

I don't see any unit tests so I assume you have been testing this manually. Can you describe your approach to testing this code, and some samples of the dry-run output you get? I'd like to work with you to get some automated test coverage in place for this behavior.

Also, we should update the documentation so people know about this behavior. Is there a place in the README or at https://capistranorb.com where it makes sense to mention it?

@edspc
Copy link
Author

edspc commented Oct 3, 2023

Hi @mattbrictson!

I've added some tests and the notice to the docs.
Please feel free to update them if necessary)

@mattbrictson
Copy link
Member

@edspc Thanks! The structure of the tests looks good. I may make some tweaks before merging, but this is very helpful.

@mattbrictson mattbrictson added the ⚠️ Breaking Introduces a backwards-incompatible change label Oct 11, 2023
@mattbrictson
Copy link
Member

@edspc I redid the specs to remove the mocks while hopefully still testing what's important. What do you think?

@mattbrictson mattbrictson changed the title Make Capistrano respect dry_run in run_locally Make Capistrano respect --dry-run in run_locally Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ Breaking Introduces a backwards-incompatible change ✨ Feature Adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants