Skip to content
This repository has been archived by the owner on Jun 2, 2019. It is now read-only.

enable maven install from package, removes maestrodev-wget dependency #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jlyheden
Copy link

and makes symlink management optional and configurable

@carlossg
Copy link
Member

Thanks! Could you leave the existing specs to test the tarball installation and add your specs as new ones?

@jlyheden
Copy link
Author

Well, part of the pull request is to remove the wget module dependency, so installation is taken care of via one exec rather than a chain of wget defines and execs. Depending on the old behaviour in the spec tests will obviously cause these tests to fail.. Or am I missing something?

@carlossg
Copy link
Member

There are some problems when using the system package, I have added the compilation tests in the jlyheden-master branch, if you run rake spec you will see test failures

What packages and in what OSes install maven? It'd be good to add an example

@jlyheden
Copy link
Author

These are in-house packaged RPMs, not that they contain anything proprietary to our org but they haven't been released to the public yet. Basically just delivering the files to /opt/apache-maven-version, same as the tar gz

@carlossg
Copy link
Member

I have merged master into the branch,

is there any reason to use File <| title == '/usr/bin/mvn' |> instead of File['/usr/bin/mvn'] declaring the dependencies?

The wget module takes care of a few things, like not using the password in the command line which would be visible to all users. Doing that here it would be a lot of duplication
I would rather use curl, but it's the same problem, and there is no puppet-curl module

@jlyheden
Copy link
Author

Since symlink management now is optional I didn't want to have a resource dependency on a resource that might not exist, using the collector syntax only applies the dependency if the symlink resource exists.

Good point about password visible in process list, that is a limitation that I did not consider. Based on some quick googling it doesn't seem to be solvable without writing a temporary wgetrc which I guess your wget module already takes care of. Personally I don't mind using the wget module, but our puppet module collection already contains a conflicting wget module so removing the maestrodev-wget dependency is rather as a workaround to puppets limitation of not supporting namespaced modules.

@carlossg
Copy link
Member

yeah, so if you want to rewrite leaving the package management and wget module I can apply.

I'd also change $system_package = false to $system_package = undef as it is misleading, seems that it expects true/false values instead of strings

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants