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.

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

Introduce an Apt::Proxy type to validate the hash #773

Merged
merged 2 commits into from
Aug 24, 2018

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Jul 26, 2018

No description provided.

@pmcmaw
Copy link

pmcmaw commented Aug 14, 2018

Hey @ekohl

Would it be possible to add some tests and documentation around this new functionality and I will work on getting it merged.

:-)

@ekohl
Copy link
Collaborator Author

ekohl commented Aug 14, 2018

describe 'proxy=' do
context 'when host=localhost' do
let(:params) { { proxy: { 'host' => 'localhost' } } }
it {
is_expected.to contain_apt__setting('conf-proxy').with(priority: '01').with_content(
%r{Acquire::http::proxy "http://localhost:8080/";},
).without_content(
%r{Acquire::https::proxy},
)
}
end
context 'when host=localhost and port=8180' do
let(:params) { { proxy: { 'host' => 'localhost', 'port' => 8180 } } }
it {
is_expected.to contain_apt__setting('conf-proxy').with(priority: '01').with_content(
%r{Acquire::http::proxy "http://localhost:8180/";},
).without_content(
%r{Acquire::https::proxy},
)
}
end
context 'when host=localhost and https=true' do
let(:params) { { proxy: { 'host' => 'localhost', 'https' => true } } }
it {
is_expected.to contain_apt__setting('conf-proxy').with(priority: '01').with_content(
%r{Acquire::http::proxy "http://localhost:8080/";},
).with_content(
%r{Acquire::https::proxy "https://localhost:8080/";},
)
}
end
context 'when host=localhost and direct=true' do
let(:params) { { proxy: { 'host' => 'localhost', 'direct' => true } } }
it {
is_expected.to contain_apt__setting('conf-proxy').with(priority: '01').with_content(
%r{Acquire::http::proxy "http://localhost:8080/";},
).with_content(
%r{Acquire::https::proxy "DIRECT";},
)
}
end
context 'when host=localhost and https=true and direct=true' do
let(:params) { { proxy: { 'host' => 'localhost', 'https' => true, 'direct' => true } } }
it {
is_expected.to contain_apt__setting('conf-proxy').with(priority: '01').with_content(
%r{Acquire::http::proxy "http://localhost:8080/";},
).with_content(
%r{Acquire::https::proxy "https://localhost:8080/";},
)
}
it {
is_expected.to contain_apt__setting('conf-proxy').with(priority: '01').with_content(
%r{Acquire::http::proxy "http://localhost:8080/";},
).without_content(
%r{Acquire::https::proxy "DIRECT";},
)
}
end
context 'when ensure=absent' do
let(:params) { { proxy: { 'ensure' => 'absent' } } }
it {
is_expected.to contain_apt__setting('conf-proxy').with(ensure: 'absent',
priority: '01')
}
end
end
already has positive tests. I was sort of trusting the Puppet type system here but I could add a negative test if desired.

Note that the same pattern of verifying hashes is used in a lot of places in this module so there's a lot more that could be replaced in the same way but I didn't have time for that.

@pmcmaw pmcmaw added the feature label Aug 14, 2018
@pmcmaw
Copy link

pmcmaw commented Aug 14, 2018

Nope testing wise thats fine @ekohl I think thats fair.
Would it be possible to add a small README entry under the defined types section.

Also thanks for the quick response! :-)

@david22swan
Copy link
Member

Have updated the readme to get this merged

@ekohl
Copy link
Collaborator Author

ekohl commented Aug 24, 2018

Thanks @david22swan. This sort of dropped of my radar for a bit. I must say I really like the Puppet 4 type system. Powerful yet straight forward to use.

@HelenCampbell HelenCampbell merged commit e99d83c into puppetlabs:master Aug 24, 2018
@HelenCampbell
Copy link

Thank for your contribution @ekohl ! The Puppet 4 type system is awesome :D

@ekohl ekohl deleted the apt-proxy-type branch September 27, 2018 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants