Skip to content
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

Merged

Conversation

neomilium
Copy link
Contributor

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'").

Copy link
Member

@ekohl ekohl left a 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.

lib/modulesync.rb Show resolved Hide resolved
lib/modulesync.rb Show resolved Hide resolved
lib/modulesync/git.rb Show resolved Hide resolved
lib/modulesync/puppet_module.rb Outdated Show resolved Hide resolved
@neomilium neomilium force-pushed the refactor-puppet-modules-properties branch 3 times, most recently from 4438fc3 to a80f0da Compare April 21, 2021 12:50
@ekohl
Copy link
Member

ekohl commented Apr 21, 2021

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.

Any thoughts on this part?

@neomilium
Copy link
Contributor Author

neomilium commented Apr 21, 2021

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.

Yes, ATM it should be PuppetModule right now as there are some specific code to manage Puppet modules. My goal with #206 is to ease maintenance and to make code more generic... but before this step I'm trying to have a more readable and clean code.
So, for sake of consistency, I started with the right name (IMHO) and will try to slit specific code later.

About managed_modules.yml filename, I would like to discuss about it too... but first, I'm focused on improving the code quality/maintainability.

Copy link
Member

@ekohl ekohl left a 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 👍

spec/unit/modulesync_spec.rb Outdated Show resolved Hide resolved
features/cli.feature Show resolved Hide resolved
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
@neomilium neomilium force-pushed the refactor-puppet-modules-properties branch from a80f0da to 900b02c Compare April 21, 2021 13:43
Copy link
Member

@ekohl ekohl left a 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.

@neomilium
Copy link
Contributor Author

Ready to merge?

@ekohl ekohl merged commit fa002a0 into voxpupuli:master Apr 21, 2021
@ekohl
Copy link
Member

ekohl commented Apr 21, 2021

I shouldn't forget to press the big green button. Kind of important ;)

@neomilium neomilium deleted the refactor-puppet-modules-properties branch April 21, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants