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

git module diff mode is excessively verbose #28319

Closed
pjnagel opened this issue Aug 17, 2017 · 7 comments
Closed

git module diff mode is excessively verbose #28319

pjnagel opened this issue Aug 17, 2017 · 7 comments
Labels
affects_2.3 This issue/PR affects Ansible v2.3 bot_closed feature This issue/PR relates to a feature request. has_pr This issue has an associated PR. module This issue/PR relates to a module. source_control Source-control category support:core This issue/PR relates to code supported by the Ansible Engineering Team.

Comments

@pjnagel
Copy link

pjnagel commented Aug 17, 2017

ISSUE TYPE
  • Feature idea
COMPONENT NAME

git module

ANSIBLE VERSION
ansible 2.3.2.0
  config file = /home/pieter.nagel/setup/lautus_ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 3.5.2 (default, Nov 17 2016, 17:05:23) [GCC 5.4.0 20160609]
SUMMARY

When running git module under ansible diff mode, the output contains a complete diff of the changes that are made to the files under source control. Depending on the number of commits, this could be huge, which is an issue for network bandwidth too.

On the other hand, not running ansible in diff mode is not always an option, because the debugging output provided by diff mode is extremely valuable.

For example, our workflow is to run ansible --check --diff at every release where out deployment playbooks have seen significant development. But all our other codebases also have a lot of commits at each release, which makes ansible --check --diff a lot less useful than it would be otherwise, and causing us to do a lot of manual pulling in various git repositories just to reduce the output spewage.

I see several different potential solutions, from simple to more complex:

  • Make the git module diff mode output more terse, i.e. something that shows you the commit SHA and commit message that is checked out afterwards, and nothing more.
  • Add a parameter to the git module whereby one can select between whether terse or full diff mode output should be used. This parameter will only have effect when running ansible under diff mode.
  • Add the ability to ansible core to add the directive 'diff_mode: no' to any task, which would override diff mode on a per task level (or maybe even per block).
  • If the tension between wanting terse versus verbose output in diff mode exists in other parts of ansible too, not just here in the git module, rework ansible's diff mode to not just be an on/off flag, but a mechanism to select terse vs. verbose diff mode. All existing modules will have same output under terse vs verbose diff mode, but a handful of modules, like git, can be incrementally changed to have different output in the two modes.
@ansibot
Copy link
Contributor

ansibot commented Aug 17, 2017

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 feature_idea module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Aug 17, 2017
@robinro
Copy link
Contributor

robinro commented Aug 17, 2017

@pjnagel Thanks for this feature request and the detailed list of suggested fixes!

I fully agree with the basic idea that the diff is too verbose for some usecases. In some other cases having the full diff in the logs is very nice.

Some comments on your suggestions (I'll call them 1-4)

  1. Nope. I'm against removing the output or even changing the default.
  2. This would be the easiest in my view. There can be a new option full_diff (or similar). This is very easy to implement.
  3. check_mode can already be (de)activated on a per-task basis (see add check_mode option for tasks #16056 for some background). Doing the same for diff_mode sounds good. Can you open a seperate issue for that? This idea is a lot more general than the git issue.
  4. In my view having --diff=1...5 would be way too complicated. Many modules don't even implement diff mode and not many have output complicated enough to warrant multiple levels.

Maybe another workaround/fix for your usecase:
5) You can use a callback plugin to filter the output and transform it to your needs. This would allow to show the diff only for some modules and increase/decrease the verbosity on the fly as well as output a custom summary for your deployment setup.

Overall I'd leave this as
needs_contributor
I see 2) or 3) as most promising solutions.

@ansibot ansibot added waiting_on_contributor This would be accepted but there are no plans to actively work on it. and removed needs_triage Needs a first human triage before being processed. labels Aug 17, 2017
@dagwieers
Copy link
Contributor

dagwieers commented Aug 17, 2017

I am responsible for the diff support, and figured it's better to have diff support (even if it was verbose) than not having it, as people can decide if they want it or not. At the same time I asked for having a per-task way to enable/disable it.

@pjnagel
Copy link
Author

pjnagel commented Aug 18, 2017

@robinro

I agree with your assessment of my proposals.

Option 2 would be of the highest value to me, since it would solve my problem while still giving me useful feedback from git module in diff mode.

If option 2 is implement, I personally won't have any immediate need for option 3, but I agree with you and @dagwieers that it would be a generally useful option.

Option 5 is technically feasible, but I am loathe to do so (other than as workaround) since it would be a very brittle solution that would required tricky regexes or something similar to filter out the unwanted part of the diff output.

Also, the diff on a large git repository can sometimes be very large (on the order of hundreds of megabytes or gigabytes), so solution 5 could impose a lot of memory pressure on a server (as could the current implementation of diff mode for the git module).

@dagwieers
Copy link
Contributor

dagwieers commented Aug 18, 2017

@pjnagel

  • There is a need to enable/disable diff output on a per task basis. (Security is one very good reason) Proposal: Controlling when diff output should be produced #14860
  • We should avoid having large diff's being returned by the module, so I would simply truncate the output if the size (or # lines) goes over a treshold. I believe we do this already somewhere, might be the template module IIRC. And maybe make this configurable for Ansible. (This is the generic solution to avoid large transfers)
  • Adding an option to influence the specific git diff output seems like a useful idea as well. When I implemented it we explicitly wanted to see the full diff as the use-case required it (and the output was always limited in size). But this option is IMO not to prevent large file transfers, just a means to get different meaningful output.

PS Don't call the option diff_mode as it's not an actual mode like run-mode/check-mode. (Internally you simply have _ansible_check_mode and _ansible_diff to make that distinction IMO) I prefer diff_when so one can influence using conditionals.

@dagwieers
Copy link
Contributor

dagwieers commented Aug 18, 2017

@pjnagel Wrt. having a terse/full output mode from within Ansible, this could be done by sending this information to the module, add some helper methods to AnsibleModule for handling diff output and letting the module take care of this.

I think there is a dire need for this, now people mess with the result dictionary themselves, but just like we now have module.warn() we should probably have a module.diff() or other helper functions.

@ansibot ansibot removed the waiting_on_contributor This would be accepted but there are no plans to actively work on it. label Oct 5, 2017
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_idea labels Mar 2, 2018
@ansibot ansibot added the waiting_on_contributor This would be accepted but there are no plans to actively work on it. label May 5, 2018
@ansibot ansibot added the source_control Source-control category label Feb 17, 2019
@ansibot ansibot added the has_pr This issue has an associated PR. label Jul 28, 2019
@ansibot
Copy link
Contributor

ansibot commented Mar 26, 2022

Thank you very much for your submission to Ansible. It means a lot to us that you've taken time to contribute.

Unfortunately, this issue has been open for some time while waiting for a contributor to take it up but there does not seem to have been anyone that did so. So we are going to close this issue to clear up the queues and make it easier for contributors to browse possible implementation targets.

However, we're absolutely always up for discussion. Because this project is very active, we're unlikely to see comments made on closed tickets and we lock them after some time. If you or anyone else has any further questions, please let us know by using any of the communication methods listed in the page below:

In the future, sometimes starting a discussion on the development list prior to proposing or implementing a feature can make getting things included a little easier, but it's not always necessary.

Thank you once again for this and your interest in Ansible!

click here for bot help

@ansibot ansibot added bot_closed and removed waiting_on_contributor This would be accepted but there are no plans to actively work on it. labels Mar 26, 2022
@ansibot ansibot closed this as completed Mar 26, 2022
@ansible ansible locked and limited conversation to collaborators Apr 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.3 This issue/PR affects Ansible v2.3 bot_closed feature This issue/PR relates to a feature request. has_pr This issue has an associated PR. module This issue/PR relates to a module. source_control Source-control category support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

No branches or pull requests

4 participants