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

feat: support for dry-run mode #88

Closed
wants to merge 10 commits into from
Closed

Conversation

mdelapenya
Copy link

@mdelapenya mdelapenya commented Nov 28, 2022

What does this PR do?

It does a few things to achieve dry-run to work properly:

  1. a way to display in a clear manner that dry-run is enabled. For that, we are adding an Infof function to the logger, which uses a new Info color. This color will display the background in Cyan and the foreground in White.
  2. a new persistent, DryRun flag, attached to the root command. This flag will be available to all subcommands.
  3. a helper function for all the cobra commands and subcommands, that will be run at the PreRun stage. This function will check if the DryRun flag is enabled, and print a message built with the previous blocks.
  4. an evaluation of the DryRun flag at all the implementations of the executors, so that the first thing that happens in there is checking if DryRun is true. If so, it will just print the command, arguments and workDir, returning without error. For the case of the ExecuteAndCapture function, it will return the very same string than the above implementation.
  5. Finally it add tests for git/github packages, which use the new code

Why is it important?

It will support testing locally changes before submitting real pull requests.

Other considerations

I probably missed some other places where the dry-run mode should be applied. Please point them out to me so that I can update the code.

Related issues

@rnorth
Copy link
Collaborator

rnorth commented Nov 29, 2022

Hi @mdelapenya, it's great to see you here 😃

Thanks for the PR - @sledigabel and I will try to review soon!

@mdelapenya
Copy link
Author

Hi @mdelapenya, it's great to see you here 😃

Thanks for the PR - @sledigabel and I will try to review soon!

Indeed, my pleasure! I'm evaluating this project to sync common files in multiple repositories. We have a home-baked shell script that does what it's expected, but looking for a more robust solution. And I wanted to give turbolift a try using dry-run mode 😄

Please ask anything you need during the review 🙏

Copy link
Collaborator

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

A few minor comments and suggestions but I like it!

cmd/internal/pre_run.go Outdated Show resolved Hide resolved
internal/executor/executor.go Outdated Show resolved Hide resolved
internal/executor/executor.go Outdated Show resolved Hide resolved
internal/github/github.go Outdated Show resolved Hide resolved
@mdelapenya
Copy link
Author

Hi @rnorth sorry for the radio silence, I went on parental leave on Dec 10th, and I was back a few weeks ago.

Returning activity on contributions at a slow pace :)

@rnorth rnorth self-assigned this May 24, 2023
@rnorth
Copy link
Collaborator

rnorth commented Jan 29, 2024

From #38:

Since this was created, #94 has come up which we think offers an alternative approach to this problem that is simpler to implement. As such, we'd like to move to close this.

Sorry that we've had this change of heart, but think that #94 would be simpler, as well as easier to integrate. Thanks for your attempt to contribute though, @mdelapenya - I really appreciate your effort.

@rnorth rnorth closed this Jan 29, 2024
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.

Allow for 'dry running' commands
2 participants