-
-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor puppet modules properties #212
Refactor puppet modules properties #212
Conversation
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.
Some minor things inline but I like this abstraction. Cleans up a lot.
Of course some bikeshedding: should it be PuppetModule
? I know it's used outside of Puppet and nothing inherently ties modulesync to Puppet modules. Perhaps ManagedModule
is a better name and in line with managed_modules.yml
.
4438fc3
to
a80f0da
Compare
Any thoughts on this part? |
Yes, ATM it should be About |
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.
Looks like a good refactor. 2 more small items, but otherwise 👍
This commit introduces a new class PuppetModule in order to simplify properties computation. This new class: - allows a single `name`, `namespace` and the working directory computation - introduces the `given_name` property, which is the name the user puts in its configuration file - adds the capability a have a longer `namespace` (e.g. puppet/modules) This commit also exposes the options through ModuleSync.options instead of passing it to almost any methods. As `name` and `namespace` can be confusing in code, so they have been renamed to `repository_name` and `repository_namespace`. This commit only introduces minor changes on the output of `msync`: some single quotes have been added as delimiters for relevant dynamic part of the messages; and the use of `given_name` (i.e. user-provided name) when relevant (e.g. "Syncing 'voxpupuli/puppet-module'").
Please note that previous commit introduced some offences that will be fixed in upcomming commits (some are already available in voxpupuli#206
a80f0da
to
900b02c
Compare
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.
Thanks! Let's get this in to allow more.
Ready to merge? |
I shouldn't forget to press the big green button. Kind of important ;) |
This commit introduces a new class PuppetModule in order to simplify properties computation.
This new class:
name
,namespace
and the working directory computationgiven_name
property, which is the name the user puts inits configuration file
namespace
(e.g. puppet/modules)This commit also exposes the options through ModuleSync.options instead
of passing it to almost any methods.
As
name
andnamespace
can be confusing in code, so they have beenrenamed to
repository_name
andrepository_namespace
.This commit only introduces minor changes on the output of
msync
: somesingle quotes have been added as delimiters for relevant dynamic part of
the messages; and the use of
given_name
(i.e. user-provided name) whenrelevant (e.g. "Syncing 'voxpupuli/puppet-module'").