Use namespace in directory structure when cloning repositories #152
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,8 +32,8 @@ def self.local_file(config_path, file) | |
| "#{config_path}/#{MODULE_FILES_DIR}/#{file}" | ||
| end | ||
|
|
||
| def self.module_file(project_root, namespace, puppet_module, file) | ||
| "#{project_root}/#{namespace}/#{puppet_module}/#{file}" | ||
| end | ||
|
|
||
| def self.local_files(path) | ||
|
|
@@ -78,16 +78,17 @@ def self.hook(options) | |
| end | ||
|
|
||
| def self.manage_file(filename, settings, options) | ||
| namespace = settings.additional_settings[:namespace] | ||
| module_name = settings.additional_settings[:puppet_module] | ||
| configs = settings.build_file_configs(filename) | ||
| if configs['delete'] | ||
| Renderer.remove(module_file(options[:project_root], namespace, module_name, filename)) | ||
| else | ||
| templatename = local_file(options[:configs], filename) | ||
| begin | ||
| erb = Renderer.build(templatename) | ||
| template = Renderer.render(erb, configs) | ||
| Renderer.sync(template, module_file(options[:project_root], namespace, module_name, filename)) | ||
| rescue # rubocop:disable Lint/RescueWithoutErrorClass | ||
| STDERR.puts "Error while rendering #{filename}" | ||
| raise | ||
|
|
@@ -101,20 +102,18 @@ def self.manage_module(puppet_module, module_files, module_options, defaults, op | |
| raise unless options[:skip_broken] | ||
| end | ||
|
|
||
| namespace, module_name = module_name(puppet_module, options[:namespace]) | ||
| git_repo = "#{namespace}/#{module_name}" | ||
|
There was a problem hiding this comment. I don't think this is correct. IMHO you should introduce a new variable There was a problem hiding this comment. Is this just a matter of taste, or a technical issue?
What do you intend with "project root"? Not the repository? Can you make a concrete example? P.S.: You can write, "Replace this by ...". I promise, I won't be offended. There was a problem hiding this comment.
I think it should be that way because the code that's called (
I meant
That sounds like I know what I'm doing :P There was a problem hiding this comment. Alight, I'm really trying -- don't get me wrong! There is a problem in the code.
And while I understand the issue with relying on Git to create subdirectories " There was a problem hiding this comment. I think this problem is artificial. When we look at the Git.pull code we see that the We can still try to prove this is false and will fail. If we manage to come up with a test case that fails. I think that's the route that makes really sense. (I rely a lot on tests, especially for languages that I don't fully master. The existing ones have helped me to get as far as I am now.) |
||
| unless options[:offline] | ||
| Git.pull(options[:git_base], git_repo, options[:branch], options[:project_root], module_options || {}) | ||
| end | ||
| module_configs = Util.parse_config("#{options[:project_root]}/#{namespace}/#{module_name}/#{MODULE_CONF_FILE}") | ||
|
There was a problem hiding this comment. This should use |
||
| settings = Settings.new(defaults[GLOBAL_DEFAULTS_KEY] || {}, | ||
| defaults, | ||
| module_configs[GLOBAL_DEFAULTS_KEY] || {}, | ||
| module_configs, | ||
| :puppet_module => module_name, | ||
| :git_base => options[:git_base], | ||
| :namespace => namespace) | ||
| settings.unmanaged_files(module_files).each do |filename| | ||
| puts "Not managing #{filename} in #{module_name}" | ||
|
|
@@ -124,10 +123,10 @@ def self.manage_module(puppet_module, module_files, module_options, defaults, op | |
| files_to_manage.each { |filename| manage_file(filename, settings, options) } | ||
|
|
||
| if options[:noop] | ||
| Git.update_noop(git_repo, options) | ||
| elsif !options[:offline] | ||
| # Git.update() returns a boolean: true if files were pushed, false if not. | ||
| pushed = Git.update(git_repo, files_to_manage, options) | ||
|
There was a problem hiding this comment. @ekohl Interestingly, even though the tests pass this may turn out to be an issue. Because in Git.update the There was a problem hiding this comment. This makes sense now, chiming in with my discovery above: The The |
||
| return nil unless pushed && options[:pr] | ||
|
|
||
| # We only do GitHub PR work if the GITHUB_TOKEN variable is set in the environment. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is correct. If a module is overridden then
module_nameextracts it from the module name rather than thenamespacesetting. Wouldn't this break ifnamespace/moduleis used?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does. Can you imagine a test case that would make the code fail?