package manager updates #47
Conversation
|
it still need to update the docs and tests... |
mcrauwel
added
enhancement
manifests/install.pp
Outdated
| 'Debian': { | ||
| ensure_packages('wget') | ||
|
|
||
| exec { 'wget-proxysql-package-source': |
There was a problem hiding this comment.
I'm not a fan of this pattern. First of all, the puppet file type can already download files if needed. Other than that you do create a security risk. Any user can write to /tmp so if they can get their .de in there before puppet runs, they can get root. Essentially this is a local root vulnerability.
The way I always look at manage_repo => false is that I have manage my own repository. This modules appears to take a slightly different route.
There was a problem hiding this comment.
you have suggestions to fix this... unfortunately dpkg does not allow remote paths for installing packages... would writing to /root be better so only root can change the file?
There was a problem hiding this comment.
That would be better, but I'm not sure I like downloading a package this way to begin with. Personally I'm a big fan of just using repositories rather than reinventing them.
There was a problem hiding this comment.
I agree with that, the $::proxysql::manage_repo variable does default to true (see this line) so this part is only called when you explicitly don't want to use upstream repo's, which I personally also not encourage...
There was a problem hiding this comment.
Indeed - let's not encourage it. Another use case for this code is that someone has an internal mirror (because they gate releases or the host has no internet access), but your generic $repo variable should give sufficient flexibility for that.
Personally I'd remove the whole wget code, but I might need to take a closer look.
There was a problem hiding this comment.
I'm fine with downloading the files. We should just not write them to a public readable directory and we should not use an exec resource. I'm not sure how good the file resource handles remote links, my suggestions is https://github.com/voxpupuli/puppet-archive
There was a problem hiding this comment.
👍 for that idea @bastelfreak
we can even add the md5 checksum to validate the downloaded file. I will implement this ASAP.
There was a problem hiding this comment.
I know for sure that at least sha1 is supported as well, can we stick with that instead of md5?
There was a problem hiding this comment.
Upstream maintainers currently only post md5 sums on the releases page. I can put in a request to also post sha1 there but as long as they don't md5 is what we are stuck with...
There was a problem hiding this comment.
@bastelfreak is this the proper way how it's done now?
|
@tullis I will try to finish work on this ASAP, although I'm preparing for a conference next week I'm doubtful if it will happen this or the next week. On the other hand you could try using the branch an test it... ;) |
|
mhm, the git history looks a bit strange here. Did you try to rebase against our latest master branch? |
|
yes I did... what looks strange? |
make repo management more generic use upstream defaults
updated docs started working on rspec tests
install depenedencies when not using the repo store downloaded file in a privileged folder
mcrauwel
force-pushed
the
proxysql-repo-official
branch
from
7c5c2af to
c89c0a6
Compare
|
@bastelfreak fixed the history issue... |
Pull Request (PR) description