git modules reports missing https password clearly

[SimonHeimberg]

SUMMARY

git module reports missing https password instead of hanging

fixes #69489

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

git

ADDITIONAL INFORMATION

Asking for the ssh password is prevented on Line411 by setting -o BatchMode=yes for ssh. To do the same for https, we can set an environment variable. Not sure if it is done in the correct place.

[samdoran]

Based on this comment it seems that setting GIT_TERMINAL_PROMPT does not fix the issue. The only way to prevent this from hanging is setting GIT_ASKPASS to something that exits cleanly.

[samdoran]

We also need a changelog entry and integration tests.

[SimonHeimberg]

Based on this comment it seems that the GIT_TERMINAL_PROMPT does not fix the issue. The only way to prevent this from hanging is setting GIT_ASKPASS to something that exits cleanly.

I tested with more git versions now.
It fails on 1.9.1, the prompt is still shown.
On git 2.7.4 and 2.11.0, it works as expected:

$ GIT_TERMINAL_PROMPT=0 git fetch
fatal: could not read Password for 'https://MyUser@github.com': terminal prompts disabled
$ echo returned $?
returned 128

So for old versions, we have to set GIT_ASKPASS=/bin/true (or bin/echo, but trying an empty password instead of the prompt argument seems safer.) When GIT_ASKPASS is set, GIT_TERMINAL_PROMPT does not have any effect. So this can be left out.

[SimonHeimberg]

We also needs a changelog entry and integration tests.

I need some guidance.
I do not find how to do this in https://docs.ansible.com/ansible/latest/community/index.html#working-with-the-ansible-repo

Where do I put the changelog?
What should the integration test do? (Test that it does not hang when the https password is not given? Or ...? I do not (yet) find any testing that no prompt is shown for ssh repos.) Will it end up in test/integration/targets/git/tasks/....?

[SimonHeimberg]

Oops, the variable name is os.environ (ment)

[samdoran]

Where do I put the changelog?

Changelog fragments go in changelogs/fragments. See this fragment as an example.

What should the integration test do? (Test that it does not hang when the https password is not given? Or ...? I do not (yet) find any testing that no prompt is shown for ssh repos.) Will it end up in test/integration/targets/git/tasks/....?

Yes, the new test would go in test/integration/taregts/git/tasks/, probably in a new tasks file. Since the tests are so complicated, we like to break that test into separate tasks files to keep things readable. Make sure to add an import_tasks task to the main.yml so the new tests run.

The test would need to set the GIT_ASKPASS environment variable in order to exercise this code path. We could look at authentication against a private repo

- name: Test failure with GIT_ASKPASS (EXPECTED FAILURE)
  git:
    repo: https://someprivate.repo
    dest: "{{ checkout_dir }}"
  environment:
    GIT_ASKPASS: /bin/false
  register: result1

- name: Test failure with GIT_ASKPASS (EXPECTED FAILURE)
  git:
    repo: https://someprivate.repo
    dest: "{{ checkout_dir }}"
  register: result2

- assert:
    that:
      - result2 is failed   

[samdoran]

The more I think about this, this would be a change in behavior that may affect some who are relying on this interactive capability in the module. It's probably uncommon, but I believe AWX is relying on this.

This should probably be a new option or an update to the documentation with an example task that shows how to set GIT_ASKPASS on the task to invoke a failure when credentials are missing.

[SimonHeimberg]

Whoever wants the interactive functionality can set GIT_TERMINAL_PROMPT=1, but they would have to do it explicitly.

Having a clear documentation (without this change) would also be a big plus to now, because this info is only found in a bug report.

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/git.py:1169:71: E261: at least two spaces before inline comment

click here for bot help

[SimonHeimberg]

TODO:

• Decide if we want to do only a doc change or finish this pull request
• when doc change:
  • find where to adapt (on https://docs.ansible.com/ansible/latest/collections/ansible/builtin/git_module.html#git-config-module there is no edit button) (probably edit description in https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/git.py)
  • update the doc, mention: to prevent blocking by https password prompt, set GIT_TERMINAL_PROMPT=0 (gives better error message) or GIT_ASKPASS=/bin/true for old git versions (before 2.3.0, see git/git@e652c0e)
• when finish pull request:
  • cleanup: add missing space before comment (git modules reports missing https password clearly #71898 (comment))
  • add changelog fragment to changelogs/fragments, mentioning GIT_TERMINAL_PROMPT=1 to disable (https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment)
  • write integration test (git modules reports missing https password clearly #71898 (comment))

[samdoran]

I think this should be a documentation change. It can be added to the EXAMPLES section in https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/git.py.

[SimonHeimberg]

so what about pullrequest #72164 ?

[samdoran]

Fixed by #72164