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

Use the read method on the return of open_url to actually get the con… #1420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gerases
Copy link

@gerases gerases commented Mar 5, 2024

Summary

Use the read method on the return of open_url to actually get the content. Otherwise the following error is returned by puppetserver:

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Function Call,
 no implicit conversion of Tempfile into String (file: <file_path>, line: 8, column: 15) on node <node_name>

@CLAassistant
Copy link

CLAassistant commented Mar 5, 2024

CLA assistant check
All committers have signed the CLA.

@gerases gerases force-pushed the fix-readjson branch 2 times, most recently from 21bf454 to cbf9c7b Compare March 6, 2024 00:05
@bastelfreak bastelfreak closed this Mar 7, 2024
@bastelfreak bastelfreak reopened this Mar 7, 2024
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

The test suite needs to be adjusted accordingly. Currently it assumes OpenURI.open_uri return a String but in reality it return a StringIO or a Tempfile depending on the response. And both have a read method.

I am not found of doubles, but something like:

diff --git a/spec/functions/loadjson_spec.rb b/spec/functions/loadjson_spec.rb
index 17d01e45..0022ebfa 100644
--- a/spec/functions/loadjson_spec.rb
+++ b/spec/functions/loadjson_spec.rb
@@ -98,7 +98,9 @@ describe 'loadjson' do
       let(:json) { '{"key":"value", {"ķęŷ":"νậŀųề" }, {"キー":"値" }' }
 
       it {
-        expect(OpenURI).to receive(:open_uri).with(filename, {}).and_return(json)
+        data = double
+        expect(data).to receive(:read).and_return(json)
+        expect(OpenURI).to receive(:open_uri).with(filename, {}).and_return(data)
         if Puppet::PUPPETVERSION[0].to_i < 8
           expect(PSON).to receive(:load).with(json).and_return(data).once
         else

Run bundle exec rake spec to run the test suite on your machine. Failures are reported here:

  • ./spec/functions/loadjson_spec.rb:100
  • ./spec/functions/loadjson_spec.rb:120
  • ./spec/functions/loadjson_spec.rb:140
  • ./spec/functions/loadjson_spec.rb:157

@gerases
Copy link
Author

gerases commented Mar 14, 2024

I am not found of doubles, but something like:

Yep, will do within a few days, thank you. Do you agree it's a bug though?

@smortex
Copy link
Collaborator

smortex commented Mar 14, 2024

Yep, will do within a few days, thank you.

Thanks!

Do you agree it's a bug though?

Yup!

@smortex smortex added the bugfix label Mar 14, 2024
@gerases gerases force-pushed the fix-readjson branch 2 times, most recently from 51fdefb to fa8339c Compare March 15, 2024 04:35
@gerases gerases requested a review from smortex March 15, 2024 04:36
@gerases
Copy link
Author

gerases commented Mar 15, 2024

@smortex, I decided to use StringIO in the tests. I'm not sure how to restart the build though.

@gerases
Copy link
Author

gerases commented Mar 15, 2024

Also I see there's no way to test using a docker image, which is a bit of a hassle. Is there a plan to add that similar to other repos of vox pupuli? By the way, why "pupuli" (which is incorrect) and not "populi"?

@bastelfreak
Copy link
Collaborator

@gerases puppetlabs/stdlib uses Litmus to spin up SUTs (systems under test), whereas Vox Pupuli uses Beaker. But as Beaker, Litmus supports Docker (and Podman I think?) Containers for SUTs. I just don't know how to use Litmus.

And Vox Pupuli is correct, it's just not 'people', but 'puppet': https://latin-dictionary.net/search/english/puppet

@gerases
Copy link
Author

gerases commented Mar 15, 2024

And Vox Pupuli is correct, it's just not 'people', but 'puppet': https://latin-dictionary.net/search/english/puppet

Oh, fantastic then. Didn't think of "pupulus". Thanks.

Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

LGTM!

@gerases
Copy link
Author

gerases commented Apr 4, 2024

@bastelfreak Could this be merged?

@bastelfreak
Copy link
Collaborator

I'm a bit hesitant on merging because I don't know if this change breaks the setup for other users. I need to test it and I currently don't have the time.

@ekohl
Copy link
Collaborator

ekohl commented Apr 5, 2024

I'm surprised I didn't link it, but #1415 was my attempt to address it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants