search

saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.15k stars 5.48k forks source link

[BUG] git state/module displays errors when looking for filter.lfs in non-existent git config #57120

Open b-a-t opened 4 years ago

b-a-t commented 4 years ago

Description We have a formula that runs git.latest state on behalf of the root user on a given machine, but this account doesn't have any custom(or any other) global git configuration. Running the formula spits a lot of [ERROR] messages in the terminal window, while formula itself finishes with the success.

# salt-call state.apply custom_minion_states
[ERROR   ] Command '['git', 'config', '--global', '--get-regexp', 'filter\\.lfs\\.']' failed with return code: 1
[ERROR   ] retcode: 1
[ERROR   ] Command '['git', 'config', '--global', '--get-regexp', 'filter\\.lfs\\.']' failed with return code: 1
[ERROR   ] retcode: 1
[ERROR   ] Command '['git', 'config', '--global', '--get-regexp', 'filter\\.lfs\\.']' failed with return code: 1
[ERROR   ] retcode: 1
[ERROR   ] Command '['git', 'config', '--global', '--get-regexp', 'filter\\.lfs\\.']' failed with return code: 1
[ERROR   ] retcode: 1
[ERROR   ] Command '['git', 'config', '--global', '--get-regexp', 'filter\\.lfs\\.']' failed with return code: 1
[ERROR   ] retcode: 1
local:
11:41:28.582288 [796.186 ms]        git.latest     Clean     Name: custom_job_1
11:41:29.462621 [ 772.21 ms]        git.latest     Clean     Name: custom_job_2
11:41:30.257556 [ 800.76 ms]        git.latest     Clean     Name: custom_job_3
...
11:41:31.204360 [806.185 ms]        git.latest     Clean     Name: custom_job_4
...
11:41:32.303571 [797.927 ms]        git.latest     Clean     Name: custom_job_5
...
Summary for local
-------------
Succeeded: 28
Failed:     0
-------------
Total states run:     28
Total run time:    4.231 s

Setup This is a simple formula, packed with git.latest stanzas, together with some file.managed. The root account which runs the formula, doesn't have ~/.gitconfig file. If an empty file is created or it just misses filter.lfs. entries - the shown error is spitted out.

Something like this should expose the problem, if there is no ~root/.gitconfig file:

custom_job_1:
  git.latest:
    - name: 'https://github.com/saltstack/salt/salt.git'
    - force_reset: True
    - target: /export/salt

Also:

# salt myminion git.config_get_regexp '^filter\.lfs\.' global=True
myminion:
    ----------
ERROR: Minions returned with non-zero exit code

and:

# salt-call git.config_get_regexp '^filter\.lfs\.' global=True
[ERROR   ] Command '['git', 'config', '--global', '--get-regexp', '^filter\\.lfs\\.']' failed with return code: 1
[ERROR   ] retcode: 1
local:
    ----------

Steps to Reproduce the behavior See above.

Expected behavior As there is no error in not having global GIT configuration file I expect not to see misleading [ERROR] messages.

Versions Report

salt-call --versions-report (Provided by running salt-call --versions-report. Please also mention any differences in master/minion versions.) ``` Salt Version: Salt: 3000.2 Dependency Versions: cffi: 1.13.2 cherrypy: Not Installed dateutil: 2.4.2 docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 2.11.1 libgit2: Not Installed M2Crypto: 0.35.2 Mako: Not Installed msgpack-pure: Not Installed msgpack-python: 0.6.2 mysql-python: Not Installed pycparser: 2.19 pycrypto: 2.6.1 pycryptodome: Not Installed pygit2: Not Installed Python: 3.6.8 (default, Aug 7 2019, 17:28:10) python-gnupg: Not Installed PyYAML: 5.3 PyZMQ: 15.3.0 smmap: Not Installed timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.1.4 System Versions: dist: centos 7.7.1908 Core locale: UTF-8 machine: x86_64 release: 3.10.0-1062.9.1.el7.x86_64 system: Linux version: CentOS Linux 7.7.1908 Core ```

Additional context It seems that at least for that particular problem the following small fix addresses the issue:

--- states/git.py.orig  2020-05-07 11:10:30.700723055 +0200
+++ states/git.py   2020-05-07 11:11:37.131826627 +0200
@@ -710,6 +710,7 @@
     use_lfs = bool(
         __salt__['git.config_get_regexp'](
             r'filter\.lfs\.',
+            ignore_retcode=True,
             **{'global': True}))
     lfs_opts = {'identity': identity} if use_lfs else {}

On a global scale all(?) the calls to git.config_get_regexp should be, possibly, re-evaluated with the absence of .gitconfig or designated pattern in mind(and that should be just simple 'None' and not an error).

waynew commented 4 years ago

@b-a-t thanks for the report!

I think this is really just because we're looking for filter.lfs always, but we shouldn't actually care, if there's no gitconfig. I'm not sure if there's a good way to tell git that we don't care in that case, or if we should go ahead and skip this step when .gitconfig doesn't exist because clearly it won't have any get-regexp stuff.

It feels like strictly ignoring the returncode here would be the wrong thing, though.

b-a-t commented 4 years ago

Meanwhile on another box I've spotted this in the logs:

2020-05-04 09:35:07,983 [salt.state       :1867][INFO    ][24278] Running state [https://github.com/Neilpang/acme.sh.git] at time 09:35:07.983567
2020-05-04 09:35:07,984 [salt.state       :1900][INFO    ][24278] Executing state git.latest for [https://github.com/Neilpang/acme.sh.git]
2020-05-04 09:35:07,985 [salt.loaded.int.module.cmdmod:397 ][INFO    ][24278] Executing command ['git', 'config', '--global', '--get-regexp', 'filter\\.lfs\\.'] in directory '/root'
2020-05-04 09:35:07,993 [salt.loaded.int.module.cmdmod:838 ][ERROR   ][24278] Command '['git', 'config', '--global', '--get-regexp', 'filter\\.lfs\\.']' failed with return code: 128
2020-05-04 09:35:07,994 [salt.loaded.int.module.cmdmod:842 ][ERROR   ][24278] stderr: fatal: $HOME not set
2020-05-04 09:35:07,994 [salt.loaded.int.module.cmdmod:844 ][ERROR   ][24278] retcode: 128

Same 3000.2, but this job was initialized by salt remotely as a part of global highstate.

In this case $HOME wasn't set either, possibly cause this is entirely remote execution, which doesn't involve common interactive SHELL execution.

b-a-t commented 4 years ago

For the git.config_get_regexp for sure we shouldn't blindly ignore the exit code. But in that particular call in git.latest state all we care about is the fact that LFS is enabled, so we can inject additional parameter for the call. Next question is how do we define, that LFS support is present? While presence of the filter.lfs.* entries in the global config can confirm, that LFS support was installed, there are cases that are not covered by this check. At minimum, LFS could be enabled on the system level(--system flag) or, only for the particular repository(--local flag). Not sure about --worktree, but I guess LFS can also be enabled there. See https://github.com/git-lfs/git-lfs/wiki/Installation#installing and https://github.com/git-lfs/git-lfs/wiki/Troubleshooting#client-side-issues for example.

So, even given check doesn't cover all the cases.

The question is - are we OK with not 100% of the coverage or not? If it's OK, then any failure to read global Git config or not finding the given regexp also should be OK and just lead to the false state for the use_lfs bool. If we want absolutely be sure that we didn't miss presence of the LFS - then we need more thorough checks.

Actually, I wonder, would it be enough to run the git lfs env in the given repository tree to verify the state of LFS?

I see, for example for the unconfigured LFS the following:

# git lfs env
git: 'lfs' is not a git command. See 'git --help'.

The most similar command is
    log

Of course, someone may configure an alias lfs just for the sake of it...

For configured LFS the output is different:

% git lfs env
git-lfs/2.10.0 (GitHub; darwin amd64; go 1.13.6)
git version 2.26.2

LocalWorkingDir=
LocalGitDir=
LocalGitStorageDir=
LocalMediaDir=lfs/objects
LocalReferenceDirs=
TempDir=lfs/tmp
ConcurrentTransfers=8
TusTransfers=false
BasicTransfersOnly=false
SkipDownloadErrors=false
FetchRecentAlways=false
FetchRecentRefsDays=7
FetchRecentCommitsDays=0
FetchRecentRefsIncludeRemotes=true
PruneOffsetDays=3
PruneVerifyRemoteAlways=false
PruneRemoteName=origin
LfsStorageDir=lfs
AccessDownload=none
AccessUpload=none
DownloadTransfers=basic,lfs-standalone-file
UploadTransfers=basic,lfs-standalone-file
GIT_EXEC_PATH=/usr/local/Cellar/git/2.26.2/libexec/git-core
git config filter.lfs.process = "git-lfs filter-process"
git config filter.lfs.smudge = "git-lfs smudge -- %f"
git config filter.lfs.clean = "git-lfs clean -- %f"

Saying all this I'd consider ignoring the retcode in the call the easiest solution :)