Use YAML.load to load classes beyond the basic types #407
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrafanie
requested review from
gmcculloug,
mkanoor and
tinaafitz
as code owners
Pull Request Test Coverage Report for Build 3557
💛 - Coveralls |
gmcculloug
reviewed
Jan 9, 2020
| @@ -104,13 +104,13 @@ | |||
| end | |||
|
|
|||
| it 'loads object from yaml' do | |||
| expect(YAML.safe_load(svc_service.to_yaml)).to eq(svc_service) | |||
| expect(YAML.load(svc_service.to_yaml)).to eq(svc_service) | |||
There was a problem hiding this comment.
Suggest we pass MiqAeMethodService::MiqAeServiceService to safe_load on this one.
There was a problem hiding this comment.
I just wanted to see you say Service Service Service 🤣
There was a problem hiding this comment.
We might need HashWithIndifferentAccess too. I'll check.
There was a problem hiding this comment.
Nope, just Service ServiceService was enough
| end | ||
|
|
||
| it 'loads invalid svc_model for objects without related ar_model' do | ||
| yaml = svc_service.to_yaml | ||
| service.delete | ||
| model_from_yaml = YAML.safe_load(yaml) | ||
| model_from_yaml = YAML.load(yaml) |
There was a problem hiding this comment.
Same here, Pass MiqAeMethodService::MiqAeServiceService to safe_load.
| @@ -280,6 +280,6 @@ def create_restart_model(script1, script2, script3) | |||
| expect(MiqQueue.count).to eq(2) | |||
| q = MiqQueue.where(:state => 'ready').first | |||
| expect(q[:server_guid]).to be_nil | |||
| expect(YAML.safe_load(q.args.first[:ae_state_data])).to eq(ae_state_data) | |||
| expect(YAML.load(q.args.first[:ae_state_data])).to eq(ae_state_data) | |||
There was a problem hiding this comment.
This one is better off using load for now.
jrafanie
force-pushed
the
fix_invalid_usage_of_yaml_safe_load
branch
from
53b4b32 to
9c9f88b
Compare
https://bugzilla.redhat.com/show_bug.cgi?id=1789153 Followup to ManageIQ/manageiq#19701 The prior behavior in core was to treat YAML.safe_load like YAML.load so let's change some of these to .load for now. We'll enumerate the list of classes where we can.
https://bugzilla.redhat.com/show_bug.cgi?id=1789153 Followup to ManageIQ/manageiq#19701 Seen when run locally or in travis output: ``` 472.50s$ bundle exec rake ** ManageIQ master, codename: Jansa [Coveralls] Set up the SimpleCov formatter. [Coveralls] Using SimpleCov's 'rails' settings. ** ManageIQ master, codename: Jansa /home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does /home/travis/build/ManageIQ/manageiq-automation_engine/vendor/bundle/gems/rspec-core-3.9.1/exe/rspec: No such file or directory - does Randomized with seed 8807 .... ```
jrafanie
force-pushed
the
fix_invalid_usage_of_yaml_safe_load
branch
from
9c9f88b to
c51ae7a
Compare
|
Checked commits jrafanie/manageiq-automation_engine@963ffa9~...c51ae7a with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 spec/engine/miq_ae_state_machine_retry_spec.rb
|
gmcculloug
approved these changes
Jan 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugzilla.redhat.com/show_bug.cgi?id=1789153
Followup to ManageIQ/manageiq#19701
The prior behavior in core was to treat YAML.safe_load like YAML.load so let's change
some of these to .load for now. We'll enumerate the list of classes where we
can.
Also, fix typo of backtick instead of quote.