Skip to content

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

Closed
gerases wants to merge 1 commit into
puppetlabs:mainfrom
gerases:fix-readjson
Closed

gerases wants to merge 1 commit into
puppetlabs:mainfrom
gerases:fix-readjson

Conversation

@gerases

@gerases gerases commented Mar 5, 2024

Copy link
Copy Markdown

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

CLAassistant commented Mar 5, 2024

Copy link
Copy Markdown

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

@smortex smortex left a comment

Copy link
Copy Markdown
Collaborator

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

gerases commented Mar 14, 2024

Copy link
Copy Markdown
Author

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

smortex commented Mar 14, 2024

Copy link
Copy Markdown
Collaborator

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

gerases commented Mar 15, 2024

Copy link
Copy Markdown
Author

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

@gerases

gerases commented Mar 15, 2024

Copy link
Copy Markdown
Author

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
Copy Markdown
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

gerases commented Mar 15, 2024

Copy link
Copy Markdown
Author

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.

@smortex smortex left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@gerases

gerases commented Apr 4, 2024

Copy link
Copy Markdown
Author

@bastelfreak Could this be merged?

@bastelfreak

Copy link
Copy Markdown
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

ekohl commented Apr 5, 2024

Copy link
Copy Markdown
Collaborator

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

@jst-cyr

jst-cyr commented Sep 4, 2026

Copy link
Copy Markdown
Member

I did a rebase to run this through the tests again with the latest main updates (similar to #1415). It looks like there was a general consensus that this was a bug but that folks were unsure how much this might break setups.

@gerases : Is this still a need in your setup?

@jst-cyr jst-cyr added the cancelled Cancelled/Closed due to inactivity, lack of need, or lack of fit label Sep 18, 2026
@jst-cyr

jst-cyr commented Sep 18, 2026

Copy link
Copy Markdown
Member

Closing this PR for now until a need rises for this in the future at which point we can re-open!

@jst-cyr jst-cyr closed this Sep 18, 2026
@gerases

gerases commented Sep 18, 2026

Copy link
Copy Markdown
Author

Sorry missed your ping, I'm not sure how much it is a problem, was just trying to contribute, i think it's pretty safe to merge, but who am i to say :)

@jst-cyr

jst-cyr commented Sep 18, 2026

Copy link
Copy Markdown
Member

Hey, no worries, @gerases ! I don't mind reopening if this is still something that people need resolved. I do see #1460 also open that seems to have some similar overlap. Do you mind taking a look at #1460 and seeing if we could centralize on that PR as the place for us to address this issue? Between @smortex and yourself, we might be able to get a solid fix here between both your attempts.

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

Labels

bugfix cancelled Cancelled/Closed due to inactivity, lack of need, or lack of fit community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants