NEW Source Control module: git add/commit/push

[lvrfrc87]

SUMMARY

New modules to add git add git commit and git push functionalities to Ansible.

This is particularly useful for example, network automation where Ansible is used in a CI/CD pipeline for template rendering and device provisioning. With this modules, all changes done in the repo during pipeline iteration (i.e. rendered template or test results) can be pushed back into the repo.

This module has been requested back in December 2018. See here: ansible/ansible#50334

https and ssh git channels are both supported.

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

- name: LOCAL -add +mode
  register: result
  git_commit:
    folder_path: "{{ playbook_dir }}/test_directory/repo"
    comment: Add .
    mode: local
    url: "{{ playbook_dir }}/test_directory/repo.git"

- name: LOCAL push on master
  register: result
  git_push:
    folder_path: "{{ playbook_dir }}/test_directory/repo"
    branch: master
    mode: local
    url: "{{ playbook_dir }}/test_directory/repo.git"

[gundalow]

As mentioned on IRC, it would be good if each subcommand could be it's own module. That would make extending the modules in the future a lot easier.

Only done a quick review. Shout out on IRC if you need help with integration tests.

[lvrfrc87]

@gundalow thanks for all comments. I will go through them and update the code according,

Regarding a module for each command. Would be git_commit (that includes git add and git commit commands) and git_push a good option? If that's ok I will "break" git_acp in 2 modules as mentioned.

[lvrfrc87]

@gundalow I have added git_commit and git_push module. I will remove git_acp once we are happy with the other 2 modules. Test are not written yet.

[felixfontein]

Try adding an empty __init__.py into plugins/modules/source_control/git/

[lvrfrc87]

@felixfontein I have added but running locally I get the same error. Also I have noticed the other folders do not have __init__.py

[felixfontein]

Hmmm, ok, then that doesn't help. I'll try it out locally later.

[felixfontein]

The ansible-docs errors are caused by flat-mapping not yet working for collections. Can you add relative symlinks to plugins/modules/ linking to both modules? That should solve that problem.

The other sanity error:

02:36 ERROR: plugins/modules/source_control/git/git_push.py:0:0: doc-required-mismatch: Argument 'url' in argument_spec is required, but is not documented as being required (75%)

is something you have to fix.

[lvrfrc87]

@felixfontein is done. I do not see CI/CD running though.

[felixfontein]

@FedericoOlivieri looks like we acidentally disabled CI. I've now reenabled it, but you need to push again before it is triggered. Maybe do a rebase to master or something like that :)

[felixfontein]

Some comments to git_commit also apply to git_push.

[felixfontein]

Suggested change

	commands.append('git -C {path} commit -m "{comment}"'.format(
	commands.append('git -C {path} commit -m "{comment}"'.format(

Never ever use strings for commands! Use lists. This will also take care of all option quoting problems. (Your current version will break if someone uses " in a comment.)

(Note that module.run_command(cmd, ...) accepts lists for cmd.)

[lvrfrc87]

Just to make sure I understand properly:

Those strings are appended to a list that is then passed to run_command

    for cmd in git_commit(module):
        _rc, output, error = module.run_command(cmd, check_rc=False)

Is not that the same?

[felixfontein]

No. Consider module.run_command('echo "Hello world!"') vs. module.run_command(['echo', 'Hello world']). Here it is clear what are parameters 1 and 2 of the command, without having to parse it first with shell lexer rules.

Also assume that the commit message is x " hello " x. Then your call would result in the arguments -m, "x ", hello, " x" (four separate ones) passed to git. Using ['-m', 'x " hello " x'] would result in precisely two arguments being passed (with message exactly as provided by user).

[lvrfrc87]

I see what you mean know. It makes absolutely sense. I will fix that

[lvrfrc87]

Regarding this bit, I have a problem to make it work.
This is the command: "cmds": ["git", "add", "1585649737984992100.txt 1585649738063034300.txt"]

When I run it against testhost I get {"changed": false, "cmd": "add", "msg": "[Errno 2] No such file or directory: b'add': b'add'", "rc": 2}

If I passe the command as string (using ' '.join()) the command is successful. I had a look around to other modules for git_config and I see they use a string for run_commands. I am bit confused :/

[felixfontein]

Just because other modules do bad things doesn't mean you should copy their behavior :) Using lists is a lot better than strings for run_command.

About that error: where exactly does that come from? Module output is not really helpful to understand what went wrong. What exactly did you pass to run_command, and what did run_command return?

[felixfontein]

What happens if output is localized?

For git commit, you should use the --porcelain option. No idea if something similar exists for git add.

[lvrfrc87]

If I use --porcelain I am going to loose all the string outputs that I use for the output and error checking. rc in git are pretty useless as it pretty much always return 1 so I believe having strings to use as anchor for if statement is a good thing. If you have an idea how to implement a safe output error check using --porcelain please let me know. Here an example of what I mean:

(NaC) olivierif:NaC federicoolivieri$ git commit  -m "ciao"
On branch dev_infra
Your branch is ahead of 'origin/dev_infra' by 3 commits.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean
(NaC) olivierif:NaC federicoolivieri$ git commit  -m "ciao"  --porcelain
(NaC) olivierif:NaC federicoolivieri$

[felixfontein]

Doing the output/error checks on string output is a terrible idea. This doesn't work at all.

Also please read man git-commit on what --porcelain does. In this case, empty output means "nothing to commit".

[lvrfrc87]

Ok. What about for git add that does not support --porcelain. Is ok to use string output or do you have other ideas? I was thinking to use rc but I am not sure if distinguish between nothing to commit, working tree clean and fatal: pathspec 'file' did not match any files

[felixfontein]

You should run git status --porcelain=v1, like here: https://github.com/buildbot/buildbot/pull/5023/files#diff-380a2f2e80216ef42112b8b625db2516R753-R761

Then you know whether there's something to commit or not.

Parsing the normal text output of git is something you should NEVER rely on. There exist translated versions of CLI programs, and your code will do the wrong things when encountering such translated output.

[felixfontein]

Failing when there's nothing to commit isn't useful in all cases. How about making that configurable?

[lvrfrc87]

Sure. I would you call the argument? allow_empty ?

[felixfontein]

Hmm, wait, I think it is better to distinguish between three behaviors:

• fail if nothing to commit;
• don't commit if there's nothing to commit;
• add empty commit if there's nothing to commit (git's --allow-empty option).

Maybe call the option empty_commit and make it an enum with values fail, skip and empty_commit. Or something similar.

(Now it also reminds me where I've seen this before: https://docs.buildbot.net/current/manual/configuration/buildsteps.html#gitcommit has an option emptyCommits. I remembered that I have actually added it because I needed it once :D Here's the PR: buildbot/buildbot#5023 that might help you with implementing this.)

[lvrfrc87]

It's ok. Good suggestion. That line you indicated though checks if there are not files to add. The commit check is the line below:

            elif 'nothing to commit, working tree clean' in output:
                module.fail_json(msg=output)

[lvrfrc87]

Developing the empty_commit=skip I realize that is not really useful in my opinion. Basically, if I am not mistaken skip mode will allow the module just to stage file and personally I cannot see a use case where you want use a specific ansible module just to add files.

[felixfontein]

empty_commit=skip means: if there's nothing to add (stage), don't commit.

[lvrfrc87]

So, empty_commit=skip would be conditional for git add command while empty_commit=allow|fail would be conditional for git commit. Is that correct? If that is the case, would not be confusing that one argument affect the behavior of 2 different thighs?

[felixfontein]

It all determines on what git commit does. Either it is potentially not run (skip), it gets the --empty-commit flag (allow), or it is run as usual (fail).

[felixfontein]

This will break if url does not starts with https://, since then remote_add has not been defined.

[lvrfrc87]

That would be just mater of indent that block under if url.startswith('https://'): Would that work for you?

    def https(path, user, token, url, branch, push_option):
        if url.startswith('https://'):
            remote_add = 'git -C {path} remote set-url origin https://{user}:{token}@{url}'.format(
                path=path,
                url=url[8:],
                user=user,
                token=token,
            )
            cmd = 'git -C {path} push origin {branch}'.format(
                path=path,
                branch=branch,
            )

            if push_option:
                index = cmd.find('origin')
                return [remote_add, cmd[:index] + '--push-option={option} '.format(option=push_option) + cmd[index:]]

            if not push_option:
                return [remote_add, cmd]

[felixfontein]

You should rather raise an error when url does not start with https://.

[felixfontein]

Also here: use lists for commands, not strings.

[felixfontein]

And add --porcelain.

[lvrfrc87]

@felixfontein I have pushed a new commit taking in account some of the point you raised. Would be great if you can have a general review. Please, note I have run just some quick integrations. Sanity have to be done.

Here some explanations of the code:

• I have divided git add and git commit in 2 separate functions where git_commit is called only if git_add is successful. I do not see the point to call git_commit if git_add does not return what we want.
• I have splitted the commands strings as you suggested. However, I could not find a way to concatenate all the commands in one list only without getting an error. This was not a big problem though as per above point, 'git_add' has to be executed separately and before git_commit
• I have added empty_commit with options: allow,fail,skip as you suggested. I have done integration for these 3 cases and they look good to me.
• I have used --porcelain without the argument v1 or v2. In my MACOS argument is not supported for --porcelain so to avoid compatibility risks, I have left the default that is v1
• I did not run git status commands. As far as my knowledge and personal experience with git can go, I believe the logic implemented gives enough safety to make sure that git add and git commit worked as expected. I saw your code example but for my level of knowledge it would take too long to reverse engineering and unfortunately I cannot spend much more time on this PR

I will review anyway all the code and especially its logic, tomorrow morning with fresh eyes.

As last note, like I said, I run out of time for this PR. if there are some minor changes to add, I will be happy to do it and update git_commit.py as well, otherwise feel free to close this PR. I will add the modules in my custom collection.

Thanks for your time anyway, is always a great opportunity to work with people of such level.

[lvrfrc87]

@felixfontein This morning I have noticed that git-commit --porcelain it does a dry run --dry-run So, in my opinion this make even more complicated the logic to check what is staged and the do the real commit.

[ansibullbot]

@lvrfrc87 this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

[lvrfrc87]

@gundalow @felixfontein I would like to discuss with you the below points before to carry-on with this PR.

• I personally do not see a great advantage in splitting the module: git add/commit/push are 3 actions always done together. I have released this module and so far is working great and people seems to like it . If your concerns are in regards to future developments, I divided the code in different functions (one for add, one for commit, one for push) so it will make easier to add new features. If you really want to have separate modules, would not make sense to have 3 modules? git_add git_commit git_push

• I spent some time around this --porcelain options and I can't get value of it: first of all, it does just a --dry-run so it would need to add the same command without --porcelain option in order to do an actual commit (not sure if the same is true for git push). Also, --porcelain return a string that needs to be evaluated (s far as I understand)

[abadger]

We've turned the bot off for now. I think I've identified why it was spamming this ticket (when it searches a ticket for whether it has already commented on it, it checks whether comments were posted by the account it's running under. This account name is hardcoded in several places and thus needs to be changed in multiple places). Fixing it, however, should be done with a little bit of refactoring (so that we only have a single point to change the data in the future). We'll likely deploy the fix and re-enable the bot next week.

[lvrfrc87]

I will close this PR and raise 2 new ones: 1 for git_commit and 1 for git_push