Use namespace in directory structure when cloning repositories #152
Conversation
|
Ruby version 2.0.0 was dropped almost 2 years ago and Ruby 2.5.1 was released this March (and is included in the latest LTS version of Ubuntu). Also the other versions have advanced, in all cases with security fixes. Should I update the .travis.yml file accordingly in this PR? |
|
I think this change makes sense but makes me wonder if we should do a major version bump. Dropping Ruby 2.0 would also be a major bump IMHO so that's nice to combine.
No, that's a different PR. Dropping 2.0 can be fine but is a larger change that would clutter it. It's also clearer when generating a changelog. |
|
The changes in this PR are really just a bugfix. Under the hood it's a structural change, yes, but there is no API change. (According to Semver this would legitimate a patch version number bump.) Is there anything I should change or add to make this PR get merged? |
|
Anything missing before this PR can be merged? |
|
While it's not an official API, we do have scripts that assume the One option I'm thinking about is to not introduce the extra layer if there's just 1 namespace, but that also creates some inconsistent behavior. Any thoughts? |
Doesn't this sound a bit like "we have a feature that exploits this bug"? -- It's a bug. So, we also need to fix those scripts. If it's not easy, because "the behavior is relied on heavily" and "outside this project", then we may need to introduce a deprecation timeline or something similar.
Should I go ahead? Are there tests for them (that allow safe refactoring)?
This is the most appropriate argument. The location of the cloned repos is a technical detail that should be hidden. Would require more time for fixing, though.
It's probably easier and cleaner to keep things simple and fix the issue with all its dependencies properly. |
They were written before the multi-namespace feature I think. That's why I suggested a major version bump so you can easily depend on the old or the new version. Note the voxpupuli config is just one. I don't know how many others have scripted around modulesync. There's another approach: we can implement a command in modulesync to print a list of directories to iterate. That way you provide a more formal way to script against and it really becomes an internal detail. |
Fine with me.
I would walk the most straight-forward route to start with, and only align the scripts mentioned above with the changes to ModuleSync in this PR. They would be small changes, which can be improved upon in a later iteration (with the approaches you mention). Is that acceptable for you? |
|
There doesn't seem to be a way to find out the ModuleSync version (both In other words, regardless to whether we take the "print a list of directories" or the "straight-forward route" approach we'll end up with scripts in the I still would prefer to get this PR merged (and the related bug fixed). This is a bit of a road blocker, because it's actually a dangerous features (since the failure remains unnoticed). |
|
@bittner with a major version bump users can require the new version in their Gemfiles. So in voxpupuli we could modify the scripts for the new version and add a |
|
I could bump the version to 1.0.0 in this PR, not a big deal. Would the PR then get merged? |
|
ping 🔔 Anyone wanting to merge this bugfix? |
|
@ekohl Since you've looked at the modulesync_config repo, would we have a problem merging it if someone released a new gem afterward, even if it was 1.0.0? I just want to make sure it's properly bound there so we don't cause problems due to bad timing of the changes between the two repos. |
|
ping 🔔 @ekohl I can apply the changes to the config repo right afterwards. Let's merge this. It's really a fix that makes sense. |
| 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 namespace_root that is the project root + namespace instead.
There was a problem hiding this comment.
Is this just a matter of taste, or a technical issue?
new variable
namespace_rootthat is the project root + namespace
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.
Is this just a matter of taste, or a technical issue?
I think it should be that way because the code that's called (Git.pull) does a Dir.mkdir(project_root) unless Dir.exist?(project_root). Your version changes to rely on git clone to create parent directories. My git (2.14.4) does this, but I don't know if all git versions do this. If we rely on git to create parent directories, we might as well remove the Dir.mkdir there. This is me trying to figure out what's going on because I don't know the project that well.
What do you intend with "project root"? Not the repository? Can you make a concrete example?
I meant namespace_root = File.join(options[:project_root], namespace) (though I'm not attached to the variable name).
P.S.: You can write, "Replace this by ...". I promise, I won't be offended.
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.
- First, "
project_root" has nothing to do with the "Git project" (of the repo being cloned locally), which would be the obvious thing to understand. In fact, oddly (or obviously when you read the code long enough), it's the root of this project. - Second, the "
namespace" does not belong to this project (but to the ones being cloned locally), hence it would in fact be irritating to combine that project root with this (dynamic) namespace. There must be a better way.
And while I understand the issue with relying on Git to create subdirectories "git_repo", on the other hand, nails the target appropriately. Let me see if I can find a solution that is better than both worlds... 🤞
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 Dir.mkdir is needed. But not for the Git operation, but because we want to do Git things in that directory.
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.)
| @@ -78,16 +78,17 @@ def self.hook(options) | |||
| end | |||
|
|
|||
| def self.manage_file(filename, settings, options) | |||
| namespace = settings.additional_settings[:namespace] | |||
There was a problem hiding this comment.
I'm not sure this is correct. If a module is overridden then module_name extracts it from the module name rather than the namespace setting. Wouldn't this break if namespace/module is used?
There was a problem hiding this comment.
I think it does. Can you imagine a test case that would make the code fail?
| elsif !options[:offline] | ||
| # Git.update() returns a boolean: true if files were pushed, false if not. | ||
| pushed = Git.update(module_name, files_to_manage, options) | ||
| 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 git_repo value is prepended with a :project_root value. Not sure, though, honestly. (Ruby is not my first language.)
There was a problem hiding this comment.
This makes sense now, chiming in with my discovery above: The :project_root has nothing to do with Git. It's just the place where all Git operations are meant to be performed in.
The Git.update only does things in the final Git project root = :project_root + Git repository path (even if this is two levels).
|
Hello from hot Zurich city center! ♨️ 🌞 Where did we leave off? Can this PR be merged as it is? (I hope it can. Otherwise let me know what should look like how, please.) |
|
Are we ready to merge this PR? Remember, it fixes a bug. 🔔 ping |
|
@ekohl can you take a look at this? |
|
Any chance this bugfix is getting merged? |
|
Rebased on latest |
There was a problem hiding this comment.
I'm still not entirely happy about this. Part of it is because of the code base itself though it's odd that the renderer is namespace aware but git isn't. It might even be cleaner to modify the git implementation to fully drop the knowledge of project_root and always pass in full paths.
| end | ||
| module_configs = Util.parse_config("#{options[:project_root]}/#{module_name}/#{MODULE_CONF_FILE}") | ||
| module_configs = Util.parse_config("#{options[:project_root]}/#{namespace}/#{module_name}/#{MODULE_CONF_FILE}") |
There was a problem hiding this comment.
This should use git_repo to stay consistent
| @@ -46,12 +46,13 @@ def self.switch_branch(repo, branch) | |||
| end | |||
|
|
|||
| def self.pull(git_base, name, branch, project_root, opts) | |||
| puts "Syncing #{name}" | |||
| Dir.mkdir(project_root) unless Dir.exist?(project_root) | |||
There was a problem hiding this comment.
This can be dropped if you're going to pass in name as dir/subdir because you're going to rely on git to create a directory anyway.
|
@ekohl I see your points and agree that some of the modules need some refactoring. For example the current implementation of the However, this is out of scope of this PR. The goal of this PR is to fix the functional defect of modulesync described in #150, while changing as little on the overall codebase as needed. |
|
Is anyone else from the project maintainers interested in merging this PR? I see that @ekohl is very involved with The Foreman. I can understand that this repo is "just a distraction". Maybe someone of you @voxpupuli people can help? I may be repeating myself, but this PR fixes an actual bug. A bug that is easily overlooked in ops business (see description). 💣 😟 |
|
Who can look at the changes in this PR and merge them---or decide to refuse them forever? @bastelfreak, @rnelson0, @ekohl, someone else? I think, we can easily define follow-up tasks to implement improvements on shortcomings of the current modules and scripts that have become apparent now. And that should be done separately to keep the change set small enough. |
There was a problem hiding this comment.
Looks good to me. @ekohl do you have any objections? Otherwise we can merge this.
|
🔔 ping |
|
Thank you for merging! 🥇 |
|
It makes sense to release a new version now, for type of bug fix. It may make sense to do a major version number jump. Though, strictly speaking, it's an internal change. The external behavior doesn't change. As discussed above, we then should look at adapting the scripts that rely on the old behavior in the modulesync_config repository. |
PR #62 added custom namespaces, but (unknowingly?) repository names had to be different across namespaces. A dangerous because almost entirely silent error, which easily gets overlooked. Details are described in issue #150.
These changes fix this limitation by cloning repositories into a
./modules/<namespace>/<repository>/directory structure (instead of only./modules/<repository>/as of today).Fixes #150
Sponsored by @vshn, The DevOps Company