voxpupuli / onceover

Your gateway drug to automated infrastructure testing with Puppet
Apache License 2.0
142 stars 45 forks source link

On non-initial spec runs, temporary control repo copied inside existing control repo rather than replacing it. #154

Closed philomory closed 6 years ago

philomory commented 6 years ago
C:\code\puppet (adam_rabbitmq_expansion)
λ ls .onceover\etc\puppetlabs\code\environments\production\
Gemfile       Rakefile                           controlrepo20180216-80288-1n0mji1  controlrepo20180216-83372-wbtpul  hiera.yaml  spec
Gemfile.lock  boltlist                           controlrepo20180216-80352-eek2sf   controlrepo20180216-84644-sdcpeo  manifests
PERCONA.md    controlrepo20180216-34876-1lbj8eb  controlrepo20180216-80808-pyfwxu   controlrepo20180216-84676-npbmst  modules
Puppetfile    controlrepo20180216-71548-1q48qvm  controlrepo20180216-80944-kzftsy   data                              scripts
README.md     controlrepo20180216-78772-d6sxfp   controlrepo20180216-82032-1th0dlm  environment.conf                  site

After the initial run,future updates fail because the copy process is copying the temp directory into the cache'd control repo, rather than overwriting the cached control repo.

I assume the offending line is this one: https://github.com/dylanratcliffe/onceover/blob/2d422bd3fa2e0a033f1a8ec3671e5cf9d6ffca2d/lib/onceover/testconfig.rb#L231

The behavior of copying a directory into another directory differs depending on whether a directory with the same name as the last segment of the target directory already exists or not: https://ruby-doc.org/stdlib-2.4.1/libdoc/fileutils/rdoc/FileUtils.html#method-c-cp_r

Copies src to dest. If src is a directory, this method copies all its contents recursively. If dest is a directory, copies src to dest/src. (Emphasis added)

In order to replace an existing directory with the same name, you first need to recursively remove that directory, and then do the copy.

philomory commented 6 years ago

Actually, ruby doc has the answer in an example:

# If you want to copy all contents of a directory instead of the
# directory itself, c.f. src/x -> dest/x, src/y -> dest/y,
# use following code.
FileUtils.cp_r 'src/.', 'dest'     # cp_r('src', 'dest') makes dest/src,
                                   # but this doesn't.

So, FileUtils.cp_r("#{temp_controlrepo}/.", "#{repo.tempdir}/#{repo.environmentpath}/production") should do it.

Though, of course, that would leave deleted files in place, so realistically you just want to delete production and then do the copy.

dylanratcliffe commented 6 years ago

Found the issue that meant tests were incorrectly passing, now to work out how to purge that cache properly. The thing that makes this hard is that I don't want to purge what r10k has done, or there is no point having the cache at all...