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

Add support for "forge_modules" in fixtures. #46

Merged
merged 1 commit into from
Apr 3, 2014

Conversation

wcooley
Copy link
Contributor

@wcooley wcooley commented Jan 24, 2014

  • This adds a new forge_modules category, similar to repositories,
    it can be given as a simple string argument, or with a hash to specify
    version.

  • I have done local testing, but I'm not sure how feasible it is to
    write unit tests for something like this. (There seem to not be any for
    existing rake tests.)

  • I ignore dependencies when installing fixture modules. Since we don't
    clean directories that are not expressly listed, we shouldn't install
    dependencies because they won't be cleaned up. Besides, it's probably
    better to have to be explicit about your chain of dependencies.

  • "repo" and "ref" don't really work as well as for git repos, but
    renaming them is outside the scope of this change and I didn't really
    want to make it any more complicated than necessary.

  • I see the direct uses of the module face in the build task, but the
    build task doesn't work for me (ironically enough, it seems like some
    of initialization that this gem supplies needs to be done):

    Could not autoload puppet/face/module/install: Error converting value for param 'modulepath': Could not find value for $confdir

In fact, this happens when I do the same thing w/irb:

irb(main):001:0> require 'puppet/face'
=> true
irb(main):002:0> pmod = Puppet::Face['module', :current]
Puppet::Error: Could not autoload puppet/face/module/install: Error converting value for param 'modulepath': Could not find value for $confdir

... so I'm just shelling out to 'puppet module install' directly.

* This adds a new *forge_modules* category, similar to *repositories*,
it can be given as a simple string argument, or with a hash to specify
version.

* I have done local testing, but I'm not sure how feasible it is to
write unit tests for something like this. (There seem to not be any for
existing rake tests.)

* I ignore dependencies when installing fixture modules.  Since we don't
clean directories that are not expressly listed, we shouldn't install
dependencies because they won't be cleaned up.  Besides, it's probably
better to have to be explicit about your chain of dependencies.

* "repo" and "ref" don't really work as well as for git repos, but
renaming them is outside the scope of this change and I didn't really
want to make it any more complicated than necessary.

* I see the direct uses of the module face in the `build` task, but the
`build` task doesn't work for me (ironically enough, it seems like some
of initialization that this gem supplies needs to be done):

    Could not autoload puppet/face/module/install: Error converting value for param 'modulepath': Could not find value for $confdir

In fact, this happens when I do the same thing w/irb:

    irb(main):001:0> require 'puppet/face'
    => true
    irb(main):002:0> pmod = Puppet::Face['module', :current]
    Puppet::Error: Could not autoload puppet/face/module/install: Error converting value for param 'modulepath': Could not find value for $confdir

... so I'm just shelling out to 'puppet module install' directly.
@electrical
Copy link

I like this feature.
This allows you to test against a stable released version.

@daenney
Copy link
Contributor

daenney commented Mar 21, 2014

Personally I don't like the extra forge_modules. Since they're always of the form username/module_name couldn't we teach repositories to recognise that instead? Perhaps we should rename repositories in that case to something like 'modules' which makes more sense anyway because we're defining what modules we need to run the tests, the repository is only where we get it from.

@wcooley
Copy link
Contributor Author

wcooley commented Mar 24, 2014

Personally I don't like the extra forge_modules. Since they're always of the form username/module_name couldn't we teach repositories to recognise that instead?

Merging distinct things increases ambiguity for a potentially more cumbersome future and, as far as I can tell, little benefit.

I also don't care for the magic required to assume that username/module_name means a Forge module. There is already enough magic in the world; I want more scientific precision.

What if, at some point, you want to add support for Mercurial or Subversion repositories, any of which could use an HTTP/S URL for source? If they're all slopped together under one category, you then have to add a vcstype parameter to differentiate them, which to be backwards compatible has to default to git (more magic). For clarity, you would then want to add this to your existing git entries, so instead of one extra line per category type, you have an extra line for every repository entry.

Perhaps we should rename repositories in that case to something like 'modules' which makes more sense anyway because we're defining what modules we need to run the tests, the repository is only where we get it from.

I would rather see the current repositories section renamed to git, to make the categorization explicit and to make the extension points obvious. I would also like to rename my own category to forge, I think. Both of which I will think more about when/if PL figures out who is going to take responsibility for this project and starts maintaining it.

Moveover, I have been thinking lately that it might be even better to use the modules defined in the Modulefile, rather than having the data duplicated in .fixtures.yml. You could keep for VCS modules for special cases, but from what I hear the published dependencies are often neglected, so unifying the source of this data might help situation overall. Neither permits testing with different combinations of module versions (or sources), which might be desirable.

hunner added a commit that referenced this pull request Apr 3, 2014
Add support for "forge_modules" in fixtures.
@hunner hunner merged commit aea0102 into puppetlabs:master Apr 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants