pyinfra icon indicating copy to clipboard operation
pyinfra copied to clipboard

[WIP] pyinfra/operations: git.config to manage system-level config

Open Pirols opened this issue 1 year ago • 1 comments

Pirols avatar Aug 27 '24 14:08 Pirols

Sorry for the long delay @Pirols - this looks great!

Hello @Fizzadar, thanks!

Just need to fix the test situation, I can't quite understand why those are failing 🤔

As you might have noticed, I have isolated the faulty (in terms of the tests' coverage) part of this PR into 9972faeb5bafd02c06aba36fa15e787945605907, which AFAICT should not be breaking any previous usage of that fact. I have not worked on this in a while, so my memory is not of much use but I had noted the following:

The problematic part is in the fixup commit and it boils down to changing the GitConfig fact to enable the collection of the system level config. The issue is raised by the unit tests and seems to be due to the updated GitConfig returning None instead of a dictionary. This is - surprisingly - happening even for tests where the system flag is not set and the command - and thus the behaviour - should go unchanged.

Now, my 2¢: git config --system works, obviously, at system level and I don't know if that requires some extra configuration in how pyinfra performs tests. In fact, git config --system --list may even output an error: fatal: unable to read config file '/etc/gitconfig': No such file or directory.

Is this of any inspiration to you?

Pirols avatar Sep 26 '24 08:09 Pirols

The problematic part is in the fixup commit and it boils down to changing the GitConfig fact to enable the collection of the system level config. The issue is raised by the unit tests and seems to be due to the updated GitConfig returning None instead of a dictionary. This is - surprisingly - happening even for tests where the system flag is not set and the command - and thus the behaviour - should go unchanged.

Argh. You're absolutely right - the problem here is that adding a new default argument to a fact breaks all the tests that use that fact since the tests reference the facts by their full arguments. So the code change is absolutely fine, but the tests now fail. Let me see if I can fix it...

Fizzadar avatar Nov 16 '24 16:11 Fizzadar

Thank you once again for this PR, @Pirols! I've fixed up the config thing which is extremely annoying feature/bug of the current test setup, unfortunately I did not manage to figure out a nicer way of doing it so just updated the tests so we can get this merged.

Lovely, thanks for taking care of this!

Pirols avatar Nov 18 '24 08:11 Pirols