Fix fetching git repository via go-getter

[zregvart]

If we point to an existing directory go-getter will try to update and
expect a git repository at that path. Given that we create a temporary
directory without any git information within it fetching will fail with
an error:

/usr/bin/git exited with 128: fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

Since go-getter expects an non-existing directory we extend the path of
the temporary directory to provide one.

[zregvart]

@robnester-rh is this the issue you encountered?

[robnester-rh]

@zregvart while this is the issue I ran into, I feel like this is a rather hacky way to deal with this. Ultimately, I'd argue that using a package that doesn't exhibit this behavior would be preferable. I'm currently working with the go-git library for git clones, etc.

[zregvart]

@zregvart while this is the issue I ran into, I feel like this is a rather hacky way to deal with this. Ultimately, I'd argue that using a package that doesn't exhibit this behavior would be preferable. I'm currently working with the go-git library for git clones, etc.

I agree that there is a slight race condition here between creating the temp directory and removing it. This could be improved by adding a nested path within dest and passing that to go-getter as the destionation.

I disagree on using another library for policy fetching, I think we want to have multiple options for fetching the policies not only git, so having one dependency that does that for us is preferable than developing a separate fetching logic for different protocols we might support.

An additional point was that this was the exact behavior of the bash scripts -- they failed in cases when the destination directory for fetching existed.

[robnester-rh]

I disagree on using another library for policy fetching, I think we want to have multiple options for fetching the policies not only git, so having one dependency that does that for us is preferable than developing a separate fetching logic for different protocols we might support.

My point was not that we should be limited solely to git, but that we should use another library that works without the need for creating, then deleting, a directory in order to do a git clone operation. I fully agree that we should be able to fetch policies from other sources / locations other than just git.

[simonbaird]

My 2 cents on the "use another library" suggestion: It seems like bike shedding to me, unless we think go-getter is seriously broken for some reason.

[robnester-rh]

An additional point was that this was the exact behavior of the bash scripts -- they failed in cases when the destination directory for fetching existed.

While that may have been the case, other libraries do not experience this issue.

My 2 cents on the "use another library" suggestion: It seems like bike shedding to me, unless we think go-getter is seriously broken for some reason.

I feel like the fact that we're having to code around an issue with go-getter that was identified in 2018 but still persists in the current v2 library, says it's broken, at least in regard to fetching with git. Other libraries make git fetching a no-brainer, whether the directory exists or not.

I completely understand the appeal of go-getter, I agree that it's nice to have the ability to fetch from multiple "targets" with one library and I didn't choose another library to be obstinate, go-getter just failed the use case. I just feel like we're coding around an issue (inability to clone into an existing directory) that is kind of a core functionality. With all of that said, if the consensus is that folks want to do that, so be it and LGTM.

[codecov]

Codecov Report

Merging #14 (fde52b4) into main (ee0577d) will increase coverage by 0.49%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
+ Coverage   46.72%   47.22%   +0.49%     
==========================================
  Files           6        6              
  Lines         214      216       +2     
==========================================
+ Hits          100      102       +2     
  Misses         92       92              
  Partials       22       22              

Impacted Files	Coverage Δ
cmd/internal/policy/source.go	70.96% <100.00%> (+2.00%)	⬆️