Rewrite loadjson to use the modern function API #1415
Conversation
ekohl
requested review from
alexjfisher,
b4ldr,
bastelfreak,
smortex,
seanmil and
a team
as code owners
lib/puppet/functions/loadjson.rb
Outdated
| # $myhash = loadjson('https://example.local/my_hash.json') | ||
| # $myhash = loadjson('https://username:password@example.local/my_hash.json') | ||
| # $myhash = loadjson('no-file.json', {'default' => 'value'}) | ||
| Puppet::Functions.create_function(:loadjson) do |
There was a problem hiding this comment.
At the same time, shouldn't we namespace this function and create a (deprecated) shim like we started with a lot of the other functions?
There was a problem hiding this comment.
I didn't consider this.
lib/puppet/functions/loadjson.rb
Outdated
| if Puppet::Util::Package.versioncmp(Puppet.version, '8.0.0').negative? | ||
| PSON.load(source) | ||
| else | ||
| JSON.load(source) |
There was a problem hiding this comment.
Rubocop is complaining here. Are you specifically using JSON.load over JSON.parse to address the bug you mentioned?
There was a problem hiding this comment.
It's copied from the old code to remain compatible.
lib/puppet/functions/loadjson.rb
Outdated
| if path.start_with?('http://', 'https://') | ||
| require 'open-uri' | ||
| begin | ||
| content = URI.open(path) { |f| load_json_source(f) } |
There was a problem hiding this comment.
What happened to all the basic authentication related code and tests?
There was a problem hiding this comment.
Good point. I assumed it would just use the credentials from the URI, but it doesn't.
Simple reproducer:
# dnf install httpd
# htpasswd -c /etc/httpd/passwords admin supersecret
# echo 'Hello World!' > /var/www/html/index.htmlNow replace Require all granted in /etc/httpd/conf/httpd.conf with the following:
AuthType Basic
AuthName "Restricted Files"
AuthUserFile "/etc/httpd/passwords"
Require valid-user
Verify it works with curl:
# curl -s -I localhost | head -n 1
HTTP/1.1 401 Unauthorized
# curl -s -I localhost -u admin:supersecret | head -n 1
HTTP/1.1 200 OK
Then we can write a small Ruby script:
require 'open-uri'
uri = URI.parse('http://admin:supersecret@localhost/index.html')
options = {}
if uri.user
options[:http_basic_authentication] = [uri.user, uri.password]
uri.user = nil
end
uri.open(**options) do |f|
puts f.read
endThis also resolves a bug where JSON.parse returned a StringIO. To properly catch this, the tests are rewritten to avoid mocking where possible. Exceptions are with external URLs and where a failure is expected.
ekohl
force-pushed
the
rewrite-loadjson-to-modern-function
branch
from
c9d26e5
to
05139bb
Compare
Summary
This also resolves a bug where JSON.parse returned a StringIO. To properly catch this, the tests are rewritten to avoid mocking where possible. Exceptions are with external URLs and where a failure is expected.
Related Issues (if any)
Fixes #1414
Checklist
puppet apply)