Add missing param declarations (Ubuntu, Windows)#16
Add missing param declarations (Ubuntu, Windows)#16edestecd merged 4 commits intoedestecd:masterfrom
Conversation
|
Note that this issue is related to STRICT_VARIABLES in the spec helper, which could be turned on as an alternative. A very similar scenario is discussed in joshuabaird/puppet-ipaclient#63 (custom facts). |
bittner
force-pushed
the
fix/add-missing-undefs
branch
13 times, most recently
from
1229509 to
e7dfc4c
Compare
|
Puppet Lab's PDK has changed opinion about supported OSes and makes test runs fail with the latest version. The second commit addresses this issue and makes Debian a supported option in general. See also: puppetlabs/pdk#338 |
|
@edestecd This PR is ready for merging. Thank you! |
bittner
force-pushed
the
fix/add-missing-undefs
branch
from
e7dfc4c to
24574e9
Compare
|
Anything I still need to do to make you merge this PR? |
| repositories: | ||
| stdlib: "https://github.com/puppetlabs/puppetlabs-stdlib.git" | ||
| apt: "https://github.com/puppetlabs/puppetlabs-apt.git" | ||
| vcsrepo: "https://github.com/puppetlabs/puppetlabs-vcsrepo.git" |
There was a problem hiding this comment.
hmm why are we switching these? It seems like puppetlabs modules still use the repos.
https://github.com/puppetlabs/puppetlabs-apache/blob/master/.fixtures.yml
I also like testing against newer code, so we can be aware sooner of upcoming changes.
There was a problem hiding this comment.
Oh, that's true! It's for testing, then it makes sense. (I thought I would do you a favor, sorry!) 😳
| source => 'https://atom.io/download/deb', | ||
| path => '/tmp/atom-amd64.deb', | ||
| extract => false, | ||
| cleanup => false, |
There was a problem hiding this comment.
Hmm I need to investigate this more. I have seen examples of downloading debs before...
Not so sure we need to keep downloading it to /tmp once it is installed. I think some other solutions I have seen solve that somehow. I need to go digging
There was a problem hiding this comment.
Also in puppet 4 the file type can take http urls as source now. If there is no extraction, we may want to use that to reduce dependencies
|
@bittner sorry for the delay. I have a few comments. |
bittner
force-pushed
the
fix/add-missing-undefs
branch
2 times, most recently
from
5d5438d to
9c27a5e
Compare
bittner
force-pushed
the
fix/add-missing-undefs
branch
from
9c27a5e to
c1fc4be
Compare
|
I hope the changes now do what they should. Thanks for your input! |
| } | ||
|
|
||
| file { '/tmp/atom-amd64.deb': | ||
| source => 'https://atom.io/download/deb', |
There was a problem hiding this comment.
On Puppet 4.8 this doesn't work for some reason. Despite the Puppet documentation saying it should work. I had to fall back to using archive for downloading the .deb file. Not sure why. I remember trying hard before giving up.
There was a problem hiding this comment.
But then again: We're using this module, this branch in production. And it works. 😏 😲
I think the world is just flat, really.
There was a problem hiding this comment.
lol, it works for us in our environment.
|
Do you have any more comments before you can merge this PR? |
|
@edestecd We want to open more PRs, and the changes in this PR would be needed to continue without fuss. Would you mind to merge it? |
bittner
force-pushed
the
fix/add-missing-undefs
branch
2 times, most recently
from
9f098fc to
cb38ce3
Compare
bittner
force-pushed
the
fix/add-missing-undefs
branch
3 times, most recently
from
4c99ba7 to
6b53b65
Compare
bittner
force-pushed
the
fix/add-missing-undefs
branch
from
6b53b65 to
bd78f5e
Compare
|
This PR now also updates the Skype installation. Now the |
| "dependencies": [ | ||
| {"name": "puppetlabs/stdlib", "version_requirement": ">= 4.2.0 < 5.0.0"}, | ||
| {"name": "puppetlabs/apt", "version_requirement": ">= 2.1.1 < 3.0.0"}, | ||
| {"name": "puppet/archive", "version_requirement": ">= 1.1.2 < 3.0.0"}, |
There was a problem hiding this comment.
I can merge if you remove this.
There was a problem hiding this comment.
Now we have a few new errors from existing code. This would go away anyway if you merge #15, I would guess.
|
The new errors are from a new rubocop check that was added today. |
|
Thanks! I'm now rebasing PR #15. Can you take a look at that later, too? |
2 participants
Tests on the roles of our control repo discovered that some param values are not set for Ubuntu and Windows. This PR adds them.