(MODULES-3037) Acceptance tests - ChocolateySource #21
Conversation
Adds negative tests for: * applying a bad manifest * trying to set a password with no user name * trying to set a uersname with no password Adds test for overwriting the source location. The default chocolatey behavior of overwriting source with the name of a resource should be discussed.
| step 'Apply Manifest' | ||
| apply_manifest(chocolatey_src, :catch_failures => true) do | ||
| step 'Verify Result' | ||
| assert_match(/Notice: \/Stage\[main\]\/Main\/Chocolateysource\[chocolatey\]\/location: location changed 'https:\/\/chocolatey.org\/api\/v2\/' to 'chocolatey'/, stdout, "stdout did not match expected") |
There was a problem hiding this comment.
@ferventcoder @Iristyle The behavior here of declaring a chcolateysource with no location and having the chocolatey.config location overwritten with the resource name seems bad. Why would you ever want that to happen? I believe it should just throw an error.
Ex: before the apply the source looks like <source id="chocolatey" value="https://chocolatey.org/api/v2/" disabled="false" priority="0" />
After it loos like <source id="chocolatey" value="chocolatey" disabled="false" priority="0" />
There was a problem hiding this comment.
The idea is to allow for:
chocolateysource { 'c:\\packages': }
There was a problem hiding this comment.
Yeah, I understand that bit. The path as title makes sense (and we saw that looking through the code).
However, if you already have a choco source defined that is not a UNC path, HTTP uri or local path on disk... then I don't think it makes sense to do something like this:
chocolateysource { 'foo': }If a source named foo exists, it sets location to foo, which is almost certainly not what you wanted - it obliterates the location with something unusable.
If you really wanted to remove foo, you would want this instead:
chocolateysource { 'foo':
ensure => absent
}Further / related... would it ever make sense for a user to do this:
chocolateysource { 'c:\\packages':
location => 'https://foo.com/'
}That's allowable as far as I can tell, but arguably doesn't make much sense, right? If that doesn't make sense, then maybe you should never modify an existing named source, when the puppet resource doesn't explicitly set a location - but you'd want to warn / notify the user probably when the title isn't a location.
There was a problem hiding this comment.
Why would someone define a resource as chocolateysource { 'foo': } like that? There are other T&P I could make the same argument on. :D
And to remove foo, I agree that's how you do it, nobody said it would be done any other way (unless I missed something).
And the third, if you wanted to have a useless name that didn't define the location very well - to put it bluntly, that's on you.
There was a problem hiding this comment.
I'm looking at this with a grain of what a customer would realistically do.
There was a problem hiding this comment.
Let's remove the behavior of defaulting location to the name. I just took a look at yumrepo and it keeps them explicit and separate.
|
This all passed locally. |
…dd_mocks (QA-2250) Add Mocks
Adds negative tests for:
Adds test for overwriting the source location. The default chocolatey behavior of overwriting source with the name
of a resource should be discussed.