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.

Sign up for GitHub

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

(MODULES-3037) Acceptance tests - ChocolateySource #21

Merged
merged 1 commit into from
Jun 1, 2016
Merged

(MODULES-3037) Acceptance tests - ChocolateySource #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions tests/reference/tests/chocolateysource/apply_manifest_without_location.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
require 'chocolatey_helper'
test_name 'MODULES-3037 - Add Source: Apply manifest without location'
confine(:to, :platform => 'windows')

backup_config

# arrange
chocolatey_src = <<-PP
chocolateysource {'chocolatey':
ensure => present,
}
PP

# teardown
teardown do
reset_config
end

# act
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")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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" />

Copy link

@ferventcoder ferventcoder Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to allow for:

chocolateysource { 'c:\\packages': }

Copy link

@Iristyle Iristyle Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@ferventcoder ferventcoder Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at this with a grain of what a customer would realistically do.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
25 changes: 25 additions & 0 deletions tests/reference/tests/chocolateysource/fail_to_appy_bad_manifest.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
require 'chocolatey_helper'
test_name 'MODULES-3037 - Add Source Sad Path: Fail to apply bad manifest'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sad path!

confine(:to, :platform => 'windows')

backup_config

# arrange
chocolatey_src = <<-PP
chocolateysource {'test':
ensure => sad,
location => 'c:\\packages',
}
PP

# teardown
teardown do
reset_config
end

# act
step 'Apply Manifest'
apply_manifest(chocolatey_src, :expect_failures => true) do
step 'Verify Failure'
assert_match(/Error: Parameter ensure failed on Chocolateysource\[test\]: Invalid value "sad"/, stderr, "stderr did not match expected")
end
26 changes: 26 additions & 0 deletions tests/reference/tests/chocolateysource/fail_to_set_password_without_user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'chocolatey_helper'
test_name 'MODULES-3037 - Add Source Sad Path: Set password with no user'
confine(:to, :platform => 'windows')

backup_config

# arrange
chocolatey_src = <<-PP
chocolateysource {'chocolatey':
ensure => present,
location => 'https://chocolatey.org/api/v2',
password => 'test',
}
PP

# teardown
teardown do
reset_config
end

# act
step 'Apply Manifest'
apply_manifest(chocolatey_src, :expect_failures => true) do
step 'Verify Failure'
assert_match(/Error: Validation of Chocolateysource\[chocolatey\] failed: If specifying user\/password, you must specify both values/, stderr, "stderr did not match expected")
end
26 changes: 26 additions & 0 deletions tests/reference/tests/chocolateysource/fail_to_set_user_without_password.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'chocolatey_helper'
test_name 'MODULES-3037 - Add Source Sad Path: Set user with no password'
confine(:to, :platform => 'windows')

backup_config

# arrange
chocolatey_src = <<-PP
chocolateysource {'chocolatey':
ensure => present,
location => 'https://chocolatey.org/api/v2',
user => 'tim',
}
PP

# teardown
teardown do
reset_config
end

# act
step 'Apply Manifest'
apply_manifest(chocolatey_src, :expect_failures => true) do
step 'Verify Failure'
assert_match(/Error: Validation of Chocolateysource\[chocolatey\] failed: If specifying user\/password, you must specify both values/, stderr, "stderr did not match expected")
end
2 changes: 1 addition & 1 deletion tests/reference/tests/chocolateysource/remove_user_pass_from_existing_source.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
step 'Verify results'
agents.each do |agent|
on(agent, "cmd.exe /c \"type #{config_file_location}\"") do |result|
assert_not_match(/tim/, get_xml_value("//sources/source[@id='chocolatey']/@user", result.output).to_s, 'User was not removed')
assert_not_match(/.+/, get_xml_value("//sources/source[@id='chocolatey']/@user", result.output).to_s, 'User was not removed')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change!

assert_not_match(/.+/, get_xml_value("//sources/source[@id='chocolatey']/@password", result.output).to_s, 'Password was not removed')
end
end