(CODEMGMT-1422) Delete spec dirs on module sync #1198
Conversation
|
This doesn't do anything to make sure that what we're deleting from is actually a Puppet module, does it? Are we still concerned about that? |
|
I was actually working on that part of the ticket now with a test PE instance, but I wasn't quite clear on why that was listed as part of the requirements for the ticket; @mwaggett do you remember why? I looked through the paper trail, but I just found the requirement, not the reason for the requirement. |
|
The reason is that people sometimes add git repos of Heira data in their puppetfiles and install them like modules, but usually with a different install path. We don't want to delete any dirs in there that might be called |
lib/r10k/module/base.rb
Outdated
| # Delete the spec dir unless @deploy_spec has been set to true | ||
| def delete_spec_dir | ||
| unless @deploy_spec | ||
| spec_path = @path + 'spec' |
There was a problem hiding this comment.
We should probably do a File.join here.
There was a problem hiding this comment.
File.join will return a string, while a <Pathname> + String operation will return a Pathname object. I could wrap the File.join in a new Pathname, but I don't think that is any better...I'm not sure I need a Pathname for the Dir.exist? and FileUtils.rm_rf calls though... 🤷
There was a problem hiding this comment.
Touche.
I didn't realize that @path was a Pathname. In that case you might want to use the instance methods defined on Pathname for symlink?, realpath, directory?...
There was a problem hiding this comment.
Oh, yeah, that sounds good to me. 👍
|
@Magisus ok, I added the moduledir detection bits now, still no tests for that specific part yet... |
| @@ -238,6 +238,7 @@ def allowed_initialize_opts | |||
| modules: :self, | |||
| cachedir: :self, | |||
| 'no-force': :self, | |||
| 'deploy-spec': :self, | |||
There was a problem hiding this comment.
Do we need a test associated with this action, like what you added here?
There was a problem hiding this comment.
I've added the tests for environment and module actions, respectively.
|
I've tested out the following scenarios:
|
tvpartytonight
force-pushed
the
CODEMGMT-1422
branch
from
603b87b to
43d8b37
Compare
| subject { described_class.new(title, dirname, {}) } | ||
|
|
||
| it 'gets deleted by default' do | ||
|
|
lib/r10k/module/base.rb
Outdated
| @@ -66,6 +71,36 @@ def full_path | |||
| path.to_s | |||
| end | |||
|
|
|||
| # Delete the spec dir unless @deploy_spec has been set to true or @spec_deletable is false | |||
| def delete_spec_dir | |||
There was a problem hiding this comment.
I think we should rename this to something like try_delete_spec_dir and then the other one can be just delete_spec_dir. Makes it more obvious what the difference is between them.
This change deletes the spec directory for deployed modules to decrease disk usage. This behavior is the new default behavior, though it can be overridden on the CLI or via a puppetfile.
tvpartytonight
force-pushed
the
CODEMGMT-1422
branch
from
43d8b37 to
6f2d88c
Compare
Please add all notable changes to the "Unreleased" section of the CHANGELOG in the format: